From 667dfd54017d223101e090e9326b2e062bc72bd5 Mon Sep 17 00:00:00 2001 From: Ludv1g Date: Wed, 22 Apr 2026 22:54:34 +0200 Subject: [PATCH 1/3] feat: Allow toggling event flag during auto-migration for empty tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, toggling `#[spacetimedb::table(event)]` on an existing table fails with `AutoMigrateError::ChangeTableEventFlag` and requires a manual migration. This PR allows the flip in either direction as a live auto-migration step when the table has zero committed rows. Non-empty tables fail with an actionable error guiding the user to clear the table first. Clients are disconnected on the flip because the change is observable to subscribers (event tables have no committed state). - **`tx_state.rs`**: New `PendingSchemaChange::TableAlterEventFlag` variant storing the old `is_event` value for rollback. - **`committed_state.rs`**: Rollback branch restores the old value on the live schema. - **`mut_tx.rs`**: New `alter_table_event_flag` — dual-write to the tx + commit table schemas and to `st_event_table`. Idempotent no-op early returns before pushing a pending change. New `delete_st_event_table_row` helper using the existing `delete_col_eq` utility (hits the unique btree index on col 0). - **`replay.rs`**: New hook on `ST_EVENT_TABLE_ID` insert/delete mirrors `reschema_table_for_st_table_update` — flips `is_event` on the referenced user-table's cached schema during replay. This is load-bearing for cold replay across the flip point. - **`relational_db.rs`**: Thin `alter_table_event_flag` wrapper. - **`auto_migrate.rs`**: `event_ok` error branch replaced with `AutoMigrateStep::ChangeEventFlag` + `ensure_disconnect_all_users`. Removed dead `AutoMigrateError::ChangeTableEventFlag` variant. - **`formatter.rs` / `termcolor_formatter.rs`**: New `format_change_event_flag` mirroring `format_change_access`. - **`update.rs`**: New `ChangeEventFlag` handler with an O(1) row-count precheck before any mutation. - **Transaction safety**: Precheck (row count) and all three writes (st_event_table, tx schema, commit schema) run in the same `MutTx`. - **Rollback**: `TableAlterEventFlag` stores the old flag value so failed txs revert `is_event` on the live schema. Idempotent flips do not push a pending change. - **Replay correctness**: Without the replay hook, cold replay from a pre-migration snapshot would miss the schema flip and post-migration inserts would silently land in committed state. The hook mirrors the existing `st_table`/`st_column` pattern. - **Client contract**: Flipping `event` changes observability — v1 subscribers stop seeing updates; v2 subscribers see a different message variant. `ensure_disconnect_all_users` forces reconnection. ``` Cannot change `event` flag on table `my_table`: table contains data. Clear the table's rows (e.g. via a reducer) before toggling the `event` annotation. ``` - [x] `cargo test -p spacetimedb-datastore --features test` — 87 pass (including 4 new `alter_table_event_flag` tests) - [x] `cargo test -p spacetimedb-schema` — 103 pass (including 3 new `change_event_flag` plan tests) - [x] `cargo test -p spacetimedb-core` — 192 pass (including 2 new empty/non-empty integration tests) - [x] `cargo clippy -p spacetimedb-datastore -p spacetimedb-schema -p spacetimedb-core --tests` clean - [x] Pre-existing event-table tests still pass (10 tests) --- crates/core/src/db/relational_db.rs | 4 + crates/core/src/db/update.rs | 130 ++++++++++++++ .../locking_tx_datastore/committed_state.rs | 5 + .../src/locking_tx_datastore/datastore.rs | 170 +++++++++++++++++- .../src/locking_tx_datastore/mut_tx.rs | 40 ++++- .../src/locking_tx_datastore/replay.rs | 33 +++- .../src/locking_tx_datastore/tx_state.rs | 4 + crates/schema/src/auto_migrate.rs | 108 ++++++++--- crates/schema/src/auto_migrate/formatter.rs | 25 +++ .../src/auto_migrate/termcolor_formatter.rs | 18 +- 10 files changed, 507 insertions(+), 30 deletions(-) diff --git a/crates/core/src/db/relational_db.rs b/crates/core/src/db/relational_db.rs index d8cd4884bcc..d11c4acd51d 100644 --- a/crates/core/src/db/relational_db.rs +++ b/crates/core/src/db/relational_db.rs @@ -989,6 +989,10 @@ impl RelationalDB { Ok(self.inner.alter_table_access_mut_tx(tx, name, access)?) } + pub(crate) fn alter_table_event_flag(&self, tx: &mut MutTx, name: &str, is_event: bool) -> Result<(), DBError> { + Ok(self.inner.alter_table_event_flag_mut_tx(tx, name, is_event)?) + } + pub(crate) fn alter_table_primary_key( &self, tx: &mut MutTx, diff --git a/crates/core/src/db/update.rs b/crates/core/src/db/update.rs index 6e7db31f7ba..1207d9d73bc 100644 --- a/crates/core/src/db/update.rs +++ b/crates/core/src/db/update.rs @@ -282,6 +282,27 @@ fn auto_migrate_database( let table_def = plan.new.stored_in_table_def(&table_name.clone().into()).unwrap(); stdb.alter_table_access(tx, table_name, table_def.table_access.into())?; } + spacetimedb_schema::auto_migrate::AutoMigrateStep::ChangeEventFlag(table_name) => { + let table_def: &TableDef = plan.new.expect_lookup(table_name); + let table_id = stdb + .table_id_from_name_mut(tx, table_name)? + .expect("ChangeEventFlag references a table that should exist"); + + // Pre-validate: flipping is only safe when the table has no committed rows. + if stdb.table_row_count_mut(tx, table_id).unwrap_or(0) > 0 { + anyhow::bail!( + "Cannot change `event` flag on table `{table_name}`: table contains data. \ + Clear the table's rows (e.g. via a reducer) before toggling the `event` annotation." + ); + } + + log!( + logger, + "Changing `event` flag on table `{table_name}` to `{}`", + table_def.is_event + ); + stdb.alter_table_event_flag(tx, table_name, table_def.is_event)?; + } spacetimedb_schema::auto_migrate::AutoMigrateStep::ChangePrimaryKey(table_name) => { let table_def = plan.new.stored_in_table_def(&table_name.clone().into()).unwrap(); log!(logger, "Changing primary key for table `{table_name}`"); @@ -339,6 +360,7 @@ mod test { db::relational_db::tests_utils::{begin_mut_tx, insert, TestDB}, host::module_host::create_table_from_def, }; + use spacetimedb_datastore::locking_tx_datastore::state_view::StateView; use spacetimedb_datastore::locking_tx_datastore::PendingSchemaChange; use spacetimedb_lib::db::raw_def::v9::{btree, RawModuleDefV9Builder, TableAccess}; use spacetimedb_sats::{product, AlgebraicType, AlgebraicType::U64}; @@ -580,4 +602,112 @@ mod test { ); Ok(()) } + + /// Build a minimal v10 module with a single user table `events` whose + /// `is_event` flag matches `is_event`. + fn single_event_table_module_v10(is_event: bool) -> ModuleDef { + use spacetimedb_lib::db::raw_def::v10::RawModuleDefV10Builder; + + let mut builder = RawModuleDefV10Builder::new(); + let t = builder.build_table_with_new_type("events", [("id", U64)], true); + let t = if is_event { t.with_event(true) } else { t }; + t.finish(); + builder + .finish() + .try_into() + .expect("should be a valid v10 module definition") + } + + #[test] + fn change_event_flag_empty_table_succeeds() -> anyhow::Result<()> { + let auth_ctx = AuthCtx::for_testing(); + let stdb = TestDB::durable()?; + + let old = single_event_table_module_v10(false); + let new = single_event_table_module_v10(true); + + // Create the non-event table. + let mut tx = begin_mut_tx(&stdb); + for def in old.tables() { + create_table_from_def(&stdb, &mut tx, &old, def)?; + } + let table_id = stdb + .table_id_from_name_mut(&tx, "events")? + .expect("table should exist"); + assert!( + !tx.get_schema(table_id) + .map(|s| s.is_event) + .expect("schema should exist"), + "fresh non-event table should have `is_event=false`" + ); + stdb.commit_tx(tx)?; + + // Apply the auto-migration flipping `is_event=true` on an empty table. + let mut tx = begin_mut_tx(&stdb); + let plan = ponder_migrate(&old, &new)?; + let res = update_database(&stdb, &mut tx, auth_ctx, plan, &TestLogger)?; + + assert!( + matches!(res, UpdateResult::RequiresClientDisconnect), + "flipping the `event` flag should disconnect clients" + ); + assert!( + tx.get_schema(table_id) + .map(|s| s.is_event) + .expect("schema should exist"), + "live schema should reflect the new `is_event=true`" + ); + assert!( + tx.pending_schema_changes() + .iter() + .any(|c| matches!(c, PendingSchemaChange::TableAlterEventFlag(_, false))), + "flipping `is_event` should record a TableAlterEventFlag pending change: {:?}", + tx.pending_schema_changes() + ); + Ok(()) + } + + #[test] + fn change_event_flag_nonempty_table_fails() -> anyhow::Result<()> { + let auth_ctx = AuthCtx::for_testing(); + let stdb = TestDB::durable()?; + + let old = single_event_table_module_v10(false); + let new = single_event_table_module_v10(true); + + // Create the table, insert a row, then attempt to flip `is_event`. + let mut tx = begin_mut_tx(&stdb); + for def in old.tables() { + create_table_from_def(&stdb, &mut tx, &old, def)?; + } + let table_id = stdb + .table_id_from_name_mut(&tx, "events")? + .expect("table should exist"); + insert(&stdb, &mut tx, table_id, &product![42u64])?; + stdb.commit_tx(tx)?; + + let mut tx = begin_mut_tx(&stdb); + let plan = ponder_migrate(&old, &new)?; + let result = update_database(&stdb, &mut tx, auth_ctx, plan, &TestLogger); + let err = result + .err() + .expect("flipping `is_event` on a non-empty table should fail"); + assert!( + err.to_string().contains("contains data"), + "error should mention that the table contains data, got: {err}" + ); + // The schema should be untouched and there should be no pending changes to roll back. + assert!( + !tx.get_schema(table_id) + .map(|s| s.is_event) + .expect("schema should exist"), + "failed migration must leave the `is_event` flag unchanged" + ); + assert!( + tx.pending_schema_changes().is_empty(), + "failed migration should leave no pending schema changes: {:?}", + tx.pending_schema_changes() + ); + Ok(()) + } } diff --git a/crates/datastore/src/locking_tx_datastore/committed_state.rs b/crates/datastore/src/locking_tx_datastore/committed_state.rs index c479be085d9..30cb597ed79 100644 --- a/crates/datastore/src/locking_tx_datastore/committed_state.rs +++ b/crates/datastore/src/locking_tx_datastore/committed_state.rs @@ -734,6 +734,11 @@ impl CommittedState { let table = self.tables.get_mut(&table_id)?; table.with_mut_schema(|s| s.table_access = access); } + // A table's `is_event` flag was changed. Change back to the old one. + TableAlterEventFlag(table_id, old_is_event) => { + let table = self.tables.get_mut(&table_id)?; + table.with_mut_schema(|s| s.is_event = old_is_event); + } // A table's primary key was changed. Change back to the old one. TableAlterPrimaryKey(table_id, old_pk) => { let table = self.tables.get_mut(&table_id)?; diff --git a/crates/datastore/src/locking_tx_datastore/datastore.rs b/crates/datastore/src/locking_tx_datastore/datastore.rs index 9d25dc144a8..43207eb7aab 100644 --- a/crates/datastore/src/locking_tx_datastore/datastore.rs +++ b/crates/datastore/src/locking_tx_datastore/datastore.rs @@ -280,6 +280,14 @@ impl Locking { tx.alter_table_access(table_id, access) } + pub fn alter_table_event_flag_mut_tx(&self, tx: &mut MutTxId, name: &str, is_event: bool) -> Result<()> { + let table_id = self + .table_id_from_name_mut_tx(tx, name)? + .ok_or_else(|| TableError::NotFound(name.into()))?; + + tx.alter_table_event_flag(table_id, is_event) + } + pub fn alter_table_primary_key_mut_tx( &self, tx: &mut MutTxId, @@ -971,7 +979,7 @@ pub(crate) mod tests { use crate::locking_tx_datastore::tx_state::PendingSchemaChange; use crate::system_tables::{ system_tables, StColumnRow, StConnectionCredentialsFields, StConstraintData, StConstraintFields, - StConstraintRow, StEventTableFields, StIndexAlgorithm, StIndexFields, StIndexRow, StRowLevelSecurityFields, + StConstraintRow, StEventTableFields, StFields as _, StIndexAlgorithm, StIndexFields, StIndexRow, StRowLevelSecurityFields, StScheduledFields, StSequenceFields, StSequenceRow, StTableRow, StVarFields, StViewArgFields, StViewFields, ST_CLIENT_ID, ST_CLIENT_NAME, ST_COLUMN_ACCESSOR_ID, ST_COLUMN_ACCESSOR_NAME, ST_COLUMN_ID, ST_COLUMN_NAME, ST_CONNECTION_CREDENTIALS_ID, ST_CONNECTION_CREDENTIALS_NAME, ST_CONSTRAINT_ID, ST_CONSTRAINT_NAME, @@ -3141,6 +3149,166 @@ pub(crate) mod tests { Ok(()) } + /// Returns whether `st_event_table` contains a row referencing `table_id`. + fn st_event_table_has_row(datastore: &Locking, tx: &MutTxId, table_id: TableId) -> bool { + datastore + .iter_by_col_eq_mut_tx( + tx, + ST_EVENT_TABLE_ID, + ColList::from(StEventTableFields::TableId.col_id()), + &table_id.into(), + ) + .expect("st_event_table lookup should succeed") + .next() + .is_some() + } + + #[test] + fn test_alter_table_event_flag_non_event_to_event() -> ResultTest<()> { + // Create a non-event table. + let (datastore, tx, table_id) = setup_table()?; + commit(&datastore, tx)?; + + // Flip `is_event` from `false` to `true`. + let mut tx = begin_mut_tx(&datastore); + assert_eq!( + tx.get_schema(table_id) + .map(|s| s.is_event) + .expect("schema should exist"), + false + ); + assert!( + !st_event_table_has_row(&datastore, &tx, table_id), + "fresh non-event table must not have a row in `st_event_table`" + ); + + tx.alter_table_event_flag(table_id, true)?; + assert_matches!( + tx.pending_schema_changes(), + &[PendingSchemaChange::TableAlterEventFlag(t, false)] if t == table_id + ); + commit(&datastore, tx)?; + + // After commit, the schema should reflect the flipped flag + // and `st_event_table` should contain the row. + let tx = begin_mut_tx(&datastore); + assert_eq!( + tx.get_schema(table_id) + .map(|s| s.is_event) + .expect("schema should exist"), + true + ); + assert!( + st_event_table_has_row(&datastore, &tx, table_id), + "after flipping to event, `st_event_table` should have the row" + ); + Ok(()) + } + + #[test] + fn test_alter_table_event_flag_event_to_non_event() -> ResultTest<()> { + // Create an event table. + let (datastore, tx, table_id) = setup_event_table()?; + commit(&datastore, tx)?; + + // Sanity check: `st_event_table` should have the row. + let mut tx = begin_mut_tx(&datastore); + assert_eq!( + tx.get_schema(table_id) + .map(|s| s.is_event) + .expect("schema should exist"), + true + ); + assert!( + st_event_table_has_row(&datastore, &tx, table_id), + "event table should have a row in `st_event_table`" + ); + + // Flip `is_event` from `true` to `false`. + tx.alter_table_event_flag(table_id, false)?; + assert_matches!( + tx.pending_schema_changes(), + &[PendingSchemaChange::TableAlterEventFlag(t, true)] if t == table_id + ); + commit(&datastore, tx)?; + + // After commit, the schema should reflect the flipped flag + // and `st_event_table` should NOT contain the row. + let tx = begin_mut_tx(&datastore); + assert_eq!( + tx.get_schema(table_id) + .map(|s| s.is_event) + .expect("schema should exist"), + false + ); + assert!( + !st_event_table_has_row(&datastore, &tx, table_id), + "after flipping to non-event, `st_event_table` should not have the row" + ); + Ok(()) + } + + #[test] + fn test_alter_table_event_flag_rollback_reverts_live_state_and_st_event_table() -> ResultTest<()> { + // Create a non-event table. + let (datastore, tx, table_id) = setup_table()?; + commit(&datastore, tx)?; + + // Start a new tx, flip, check pending change, then rollback. + let mut tx = begin_mut_tx(&datastore); + assert!(!st_event_table_has_row(&datastore, &tx, table_id)); + + tx.alter_table_event_flag(table_id, true)?; + assert_matches!( + tx.pending_schema_changes(), + &[PendingSchemaChange::TableAlterEventFlag(t, false)] if t == table_id + ); + // The in-tx view must reflect the flip. + assert_eq!( + tx.get_schema(table_id) + .map(|s| s.is_event) + .expect("schema should exist"), + true + ); + assert!( + st_event_table_has_row(&datastore, &tx, table_id), + "after flipping within the tx, `st_event_table` should have the row" + ); + let _ = datastore.rollback_mut_tx(tx); + + // After rollback, the schema and `st_event_table` should be back to pre-state. + let tx = begin_mut_tx(&datastore); + assert!( + tx.pending_schema_changes().is_empty(), + "rollback should clear pending schema changes" + ); + assert_eq!( + tx.get_schema(table_id) + .map(|s| s.is_event) + .expect("schema should exist"), + false, + "rollback should revert `is_event` to its pre-tx value" + ); + assert!( + !st_event_table_has_row(&datastore, &tx, table_id), + "rollback should revert the `st_event_table` row" + ); + Ok(()) + } + + #[test] + fn test_alter_table_event_flag_idempotent_no_pending_change() -> ResultTest<()> { + // Create a non-event table. + let (datastore, tx, table_id) = setup_table()?; + commit(&datastore, tx)?; + + // Flipping to the same value must be a no-op with no pending change. + let mut tx = begin_mut_tx(&datastore); + tx.alter_table_event_flag(table_id, false)?; + assert_matches!(tx.pending_schema_changes(), &[]); + Ok(()) + } + #[test] fn test_alter_table_row_type_rejects_some_bad_changes() -> ResultTest<()> { let datastore = get_datastore()?; diff --git a/crates/datastore/src/locking_tx_datastore/mut_tx.rs b/crates/datastore/src/locking_tx_datastore/mut_tx.rs index d73f794e029..9b3813b6b9f 100644 --- a/crates/datastore/src/locking_tx_datastore/mut_tx.rs +++ b/crates/datastore/src/locking_tx_datastore/mut_tx.rs @@ -20,7 +20,7 @@ use crate::{ error::{IndexError, SequenceError, TableError}, system_tables::{ with_sys_table_buf, StClientFields, StClientRow, StColumnAccessorFields, StColumnAccessorRow, StColumnFields, - StColumnRow, StConstraintFields, StConstraintRow, StEventTableRow, StFields as _, StIndexAccessorFields, + StColumnRow, StConstraintFields, StConstraintRow, StEventTableFields, StEventTableRow, StFields as _, StIndexAccessorFields, StIndexAccessorRow, StIndexFields, StIndexRow, StRowLevelSecurityFields, StRowLevelSecurityRow, StScheduledFields, StScheduledRow, StSequenceFields, StSequenceRow, StTableAccessorFields, StTableAccessorRow, StTableFields, StTableRow, SystemTable, ST_CLIENT_ID, ST_COLUMN_ACCESSOR_ID, ST_COLUMN_ID, ST_CONSTRAINT_ID, @@ -1082,6 +1082,44 @@ impl MutTxId { Ok(()) } + /// Change the `is_event` flag of the table identified by `table_id`. + /// + /// Updates both the in-memory schema and the `st_event_table` system table. + /// This is a breaking change for subscribed clients (the committed state + /// semantics of the table flip), so callers must arrange a `DisconnectAllUsers`. + pub(crate) fn alter_table_event_flag(&mut self, table_id: TableId, is_event: bool) -> Result<()> { + // Write to the table in the tx state (and clone into commit state). + let ((tx_table, ..), (commit_table, ..)) = self.get_or_create_insert_table_mut(table_id)?; + let old_is_event = tx_table.get_schema().is_event; + if old_is_event == is_event { + // Idempotent no-op; do not record a pending change or it would confuse rollback. + return Ok(()); + } + tx_table.with_mut_schema_and_clone(commit_table, |s| s.is_event = is_event); + + // Update `st_event_table`. + if is_event { + let row = StEventTableRow { table_id }; + self.insert_via_serialize_bsatn(ST_EVENT_TABLE_ID, &row)?; + } else { + self.delete_st_event_table_row(table_id)?; + } + + // Remember the pending change so we can undo if necessary. + self.push_schema_change(PendingSchemaChange::TableAlterEventFlag(table_id, old_is_event)); + + Ok(()) + } + + /// Drops the row in `st_event_table` for this `table_id`. + fn delete_st_event_table_row(&mut self, table_id: TableId) -> Result<()> { + self.delete_col_eq( + ST_EVENT_TABLE_ID, + StEventTableFields::TableId.col_id(), + &table_id.into(), + ) + } + /// Change the primary key of the table identified by `table_id`. /// /// Updates both the in-memory schema and the `st_table` system table. diff --git a/crates/datastore/src/locking_tx_datastore/replay.rs b/crates/datastore/src/locking_tx_datastore/replay.rs index cf0fd357f39..47a6194607c 100644 --- a/crates/datastore/src/locking_tx_datastore/replay.rs +++ b/crates/datastore/src/locking_tx_datastore/replay.rs @@ -5,8 +5,8 @@ use crate::error::{DatastoreError, IndexError, TableError, ViewError}; use crate::locking_tx_datastore::state_view::{iter_st_column_for_table, StateView}; use crate::system_tables::{ is_built_in_meta_row, table_id_is_reserved, StColumnRow, StConstraintData, StConstraintRow, StFields as _, - StIndexRow, StSequenceFields, StTableFields, StTableRow, StViewRow, ST_COLUMN_ID, ST_CONSTRAINT_ID, ST_INDEX_ID, - ST_SEQUENCE_ID, ST_TABLE_ID, ST_VIEW_ARG_ID, ST_VIEW_ID, ST_VIEW_SUB_ID, + StIndexRow, StSequenceFields, StTableFields, StTableRow, StViewRow, ST_COLUMN_ID, ST_CONSTRAINT_ID, + ST_EVENT_TABLE_ID, ST_INDEX_ID, ST_SEQUENCE_ID, ST_TABLE_ID, ST_VIEW_ARG_ID, ST_VIEW_ID, ST_VIEW_SUB_ID, }; use anyhow::{anyhow, Context}; use core::cell::RefMut; @@ -762,6 +762,14 @@ impl<'cs> ReplayCommittedState<'cs> { self.st_column_changed(referenced_table_id)?; } + if table_id == ST_EVENT_TABLE_ID { + // An `st_event_table` row was inserted; flip `is_event=true` + // on the referenced table's cached schema. + // The `table_id` is the first (and only) field in `StEventTableRow`. + let referenced_table_id = Self::read_table_id(row); + self.reschema_table_for_st_event_table_update(referenced_table_id, true); + } + Ok(()) } @@ -807,6 +815,19 @@ impl<'cs> ReplayCommittedState<'cs> { Ok(()) } + /// Update the in-memory table structure's `is_event` flag in response to + /// replay of an `st_event_table` mutation. + fn reschema_table_for_st_event_table_update(&mut self, table_id: TableId, is_event: bool) { + // We only need to update if we've already constructed the in-memory table structure. + // If we haven't yet, then `self.get_table_and_blob_store_or_create` will see the correct schema + // (via `schema_for_table_raw`'s live `st_event_table` lookup) when it eventually runs. + if let Ok((table, ..)) = self.get_table_and_blob_store_mut(table_id) { + table.with_mut_schema(|schema| { + schema.is_event = is_event; + }); + } + } + /// Mark all `st_column` rows which refer to the same column as `st_column_row` /// other than the one at `row_pointer` as outdated /// by storing them in [`Self::replay_columns_to_ignore`]. @@ -936,6 +957,14 @@ impl<'cs> ReplayCommittedState<'cs> { self.replay_columns_to_ignore.remove(&row_ptr); } + if table_id == ST_EVENT_TABLE_ID { + // An `st_event_table` row was deleted; flip `is_event=false` + // on the referenced table's cached schema. + // The `table_id` is the first (and only) field in `StEventTableRow`. + let referenced_table_id = Self::read_table_id(row); + self.reschema_table_for_st_event_table_update(referenced_table_id, false); + } + Ok(()) } diff --git a/crates/datastore/src/locking_tx_datastore/tx_state.rs b/crates/datastore/src/locking_tx_datastore/tx_state.rs index 4e712bcf684..7d13180647c 100644 --- a/crates/datastore/src/locking_tx_datastore/tx_state.rs +++ b/crates/datastore/src/locking_tx_datastore/tx_state.rs @@ -118,6 +118,9 @@ pub enum PendingSchemaChange { /// The access of the table with [`TableId`] was changed. /// The old access was stored. TableAlterAccess(TableId, StAccess), + /// The `is_event` flag of the table with [`TableId`] was changed. + /// The old value is stored. + TableAlterEventFlag(TableId, bool), /// The row type of the table with [`TableId`] was changed. /// The old column schemas was stored. /// Only non-representational row-type changes are allowed here, @@ -148,6 +151,7 @@ impl MemoryUsage for PendingSchemaChange { Self::TableRemoved(table_id, table) => table_id.heap_usage() + table.heap_usage(), Self::TableAdded(table_id) => table_id.heap_usage(), Self::TableAlterAccess(table_id, st_access) => table_id.heap_usage() + st_access.heap_usage(), + Self::TableAlterEventFlag(table_id, old_is_event) => table_id.heap_usage() + old_is_event.heap_usage(), Self::TableAlterRowType(table_id, column_schemas) => table_id.heap_usage() + column_schemas.heap_usage(), Self::TableAlterPrimaryKey(table_id, pk) => table_id.heap_usage() + pk.heap_usage(), Self::ConstraintRemoved(table_id, constraint_schema) => { diff --git a/crates/schema/src/auto_migrate.rs b/crates/schema/src/auto_migrate.rs index 6d169a902cc..6fce9345600 100644 --- a/crates/schema/src/auto_migrate.rs +++ b/crates/schema/src/auto_migrate.rs @@ -301,6 +301,13 @@ pub enum AutoMigrateStep<'def> { /// Change the access of a table. ChangeAccess(::Key<'def>), + /// Toggle the `is_event` flag of a table. The table must be empty at apply time. + /// + /// This is a breaking change for subscribed clients (the committed-state + /// semantics of the table flip), so this step is always accompanied by a + /// `DisconnectAllUsers`. + ChangeEventFlag(::Key<'def>), + /// Change the primary key of a table. /// /// This updates the `table_primary_key` field in `st_table` @@ -434,9 +441,6 @@ pub enum AutoMigrateError { type2: TableType, }, - #[error("Changing the event flag of table {table} requires a manual migration")] - ChangeTableEventFlag { table: Identifier }, - #[error( "Changing the accessor name on index {index} from {old_accessor:?} to {new_accessor:?} requires a manual migration" )] @@ -674,14 +678,12 @@ fn auto_migrate_table<'def>(plan: &mut AutoMigratePlan<'def>, old: &'def TableDe } .into()) }; - let event_ok: Result<()> = if old.is_event == new.is_event { - Ok(()) - } else { - Err(AutoMigrateError::ChangeTableEventFlag { - table: old.name.clone(), - } - .into()) - }; + if old.is_event != new.is_event { + // Flipping `is_event` changes committed-state semantics; + // clients must reconnect after the migration. + plan.ensure_disconnect_all_users(); + plan.steps.push(AutoMigrateStep::ChangeEventFlag(key)); + } if old.table_access != new.table_access { plan.steps.push(AutoMigrateStep::ChangeAccess(key)); } @@ -760,8 +762,8 @@ fn auto_migrate_table<'def>(plan: &mut AutoMigratePlan<'def>, old: &'def TableDe }) .collect_all_errors::>(); - let ((), (), ProductMonoid(Any(row_type_changed), Any(columns_added))) = - (type_ok, event_ok, columns_ok).combine_errors()?; + let ((), ProductMonoid(Any(row_type_changed), Any(columns_added))) = + (type_ok, columns_ok).combine_errors()?; // If we're adding a column, we'll rewrite the whole table. // That makes any `ChangeColumns` moot, so we can skip it. @@ -2391,11 +2393,11 @@ mod tests { } #[test] - fn test_change_event_flag_rejected() { + fn test_change_event_flag_produces_step_non_to_event() { // non-event → event let old = create_v10_module_def(|builder| { builder - .build_table_with_new_type("Events", ProductType::from([("id", AlgebraicType::U64)]), true) + .build_table_with_new_type("events", ProductType::from([("id", AlgebraicType::U64)]), true) .finish(); }); let new = create_v10_module_def(|builder| { @@ -2405,17 +2407,75 @@ mod tests { .finish(); }); - let result = ponder_auto_migrate(&old, &new); - expect_error_matching!( - result, - AutoMigrateError::ChangeTableEventFlag { table } => &table[..] == "events" + let events_key = expect_identifier("events"); + let plan = ponder_auto_migrate(&old, &new).expect("toggling `is_event` on an empty table should succeed"); + assert_eq!( + plan.steps, + &[ + AutoMigrateStep::ChangeEventFlag(&events_key), + AutoMigrateStep::DisconnectAllUsers, + ], ); + assert!(plan.disconnects_all_users(), "{plan:#?}"); + } - // event → non-event (reverse direction) - let result = ponder_auto_migrate(&new, &old); - expect_error_matching!( - result, - AutoMigrateError::ChangeTableEventFlag { table } => &table[..] == "events" + #[test] + fn test_change_event_flag_produces_step_event_to_non() { + // event → non-event + let old = create_v10_module_def(|builder| { + builder + .build_table_with_new_type("events", ProductType::from([("id", AlgebraicType::U64)]), true) + .with_event(true) + .finish(); + }); + let new = create_v10_module_def(|builder| { + builder + .build_table_with_new_type("events", ProductType::from([("id", AlgebraicType::U64)]), true) + .finish(); + }); + + let events_key = expect_identifier("events"); + let plan = ponder_auto_migrate(&old, &new).expect("toggling `is_event` on an empty table should succeed"); + assert_eq!( + plan.steps, + &[ + AutoMigrateStep::ChangeEventFlag(&events_key), + AutoMigrateStep::DisconnectAllUsers, + ], + ); + assert!(plan.disconnects_all_users(), "{plan:#?}"); + } + + #[test] + fn test_change_event_flag_does_not_produce_orphan_sub_object_steps() { + // Flipping `is_event` must not trigger spurious index/constraint/sequence diff steps + // for a table whose only change is the `is_event` annotation. + let old = create_v10_module_def(|builder| { + builder + .build_table_with_new_type("events", ProductType::from([("id", AlgebraicType::U64)]), true) + .with_unique_constraint(0) + .with_index(btree(0), "events_id_idx", "events_id_idx") + .with_primary_key(0) + .finish(); + }); + let new = create_v10_module_def(|builder| { + builder + .build_table_with_new_type("events", ProductType::from([("id", AlgebraicType::U64)]), true) + .with_unique_constraint(0) + .with_index(btree(0), "events_id_idx", "events_id_idx") + .with_primary_key(0) + .with_event(true) + .finish(); + }); + + let events_key = expect_identifier("events"); + let plan = ponder_auto_migrate(&old, &new).expect("toggling `is_event` on an empty table should succeed"); + assert_eq!( + plan.steps, + &[ + AutoMigrateStep::ChangeEventFlag(&events_key), + AutoMigrateStep::DisconnectAllUsers, + ], ); } diff --git a/crates/schema/src/auto_migrate/formatter.rs b/crates/schema/src/auto_migrate/formatter.rs index 1f7377209bf..cc16a4ee938 100644 --- a/crates/schema/src/auto_migrate/formatter.rs +++ b/crates/schema/src/auto_migrate/formatter.rs @@ -75,6 +75,10 @@ fn format_step( let access_info = extract_access_change_info(*table, plan)?; f.format_change_access(&access_info) } + AutoMigrateStep::ChangeEventFlag(table) => { + let event_info = extract_event_flag_change_info(*table, plan)?; + f.format_change_event_flag(&event_info) + } AutoMigrateStep::ChangePrimaryKey(table) => { let old_table = plan.old.lookup_expect::(*table); let new_table = plan.new.lookup_expect::(*table); @@ -156,6 +160,7 @@ pub trait MigrationFormatter { fn format_constraint(&mut self, constraint_info: &ConstraintInfo, action: Action) -> io::Result<()>; fn format_sequence(&mut self, sequence_info: &SequenceInfo, action: Action) -> io::Result<()>; fn format_change_access(&mut self, access_info: &AccessChangeInfo) -> io::Result<()>; + fn format_change_event_flag(&mut self, event_info: &EventFlagChangeInfo) -> io::Result<()>; fn format_change_primary_key( &mut self, table_name: &str, @@ -235,6 +240,12 @@ pub struct AccessChangeInfo { pub new_access: TableAccess, } +#[derive(Debug, Clone, PartialEq)] +pub struct EventFlagChangeInfo { + pub table_name: Identifier, + pub new_is_event: bool, +} + #[derive(Debug, Clone, PartialEq)] pub struct ScheduleInfo { pub table_name: Identifier, @@ -513,6 +524,20 @@ fn extract_access_change_info( }) } +fn extract_event_flag_change_info( + table: ::Key<'_>, + plan: &super::AutoMigratePlan, +) -> Result { + let table_def = plan.new.table(table).ok_or_else(|| FormattingErrors::TableNotFound { + table: table.to_string().into(), + })?; + + Ok(EventFlagChangeInfo { + table_name: table_def.name.clone(), + new_is_event: table_def.is_event, + }) +} + fn extract_schedule_info( schedule_table: ::Key<'_>, module_def: &ModuleDef, diff --git a/crates/schema/src/auto_migrate/termcolor_formatter.rs b/crates/schema/src/auto_migrate/termcolor_formatter.rs index 31807d14b30..f08532d70a8 100644 --- a/crates/schema/src/auto_migrate/termcolor_formatter.rs +++ b/crates/schema/src/auto_migrate/termcolor_formatter.rs @@ -10,8 +10,8 @@ use crate::auto_migrate::formatter::ViewInfo; use crate::identifier::Identifier; use super::formatter::{ - AccessChangeInfo, Action, ColumnChange, ColumnChanges, ConstraintInfo, IndexInfo, MigrationFormatter, NewColumns, - RlsInfo, ScheduleInfo, SequenceInfo, TableInfo, + AccessChangeInfo, Action, ColumnChange, ColumnChanges, ConstraintInfo, EventFlagChangeInfo, IndexInfo, + MigrationFormatter, NewColumns, RlsInfo, ScheduleInfo, SequenceInfo, TableInfo, }; /// Color scheme for consistent formatting @@ -325,6 +325,20 @@ impl MigrationFormatter for TermColorFormatter { self.buffer.write_all(b")\n") } + fn format_change_event_flag(&mut self, e: &EventFlagChangeInfo) -> io::Result<()> { + let direction = if e.new_is_event { + "non-event → event" + } else { + "event → non-event" + }; + self.write_action_prefix(&Action::Changed)?; + self.buffer.write_all(b" event flag for table ")?; + self.write_colored(&e.table_name, Some(self.colors.table_name), true)?; + self.buffer.write_all(b" (")?; + self.write_colored(direction, Some(self.colors.access), false)?; + self.buffer.write_all(b")\n") + } + fn format_change_primary_key( &mut self, table_name: &str, From ead511c3f4e419c28b16879f5e6475eba3ac1c9c Mon Sep 17 00:00:00 2001 From: Ludv1g Date: Thu, 23 Apr 2026 22:45:24 +0200 Subject: [PATCH 2/3] refactor(event-flag): apply Centril 2026-04-23 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address all 16 actionable items from the review — deferred item D1 (moving the row-count precheck into `AutoMigratePrecheck::CheckTableEmpty`) is not addressed here and will follow in a separate PR as agreed. - `schema/auto_migrate.rs`: merge `test_change_event_flag_produces_step_{non_to_event,event_to_non}` into a single test with a closure called twice; make the sub-object-steps test use the same closure pattern instead of duplicating the old/new builder with an implicit `false` vs explicit `true`. - `datastore/replay.rs`: assert `num_rows()==0` inside the `reschema_table_for_st_event_table_update` hook. This enforces the feature's core invariant during cold replay. - `datastore/committed_state.rs`: same assertion on the `TableAlterEventFlag` rollback arm. - `datastore/mut_tx.rs`: extract `insert_st_event_table_row(table_id)` shared by `create_table` and `alter_table_event_flag`. - `datastore/locking_tx_datastore/test_helpers.rs` (new): host the cross-crate-reachable `assert_is_event_state`, `st_event_table_has_row`, and `check_table_event_flag_altered` helpers. Gated by the existing `test` feature. Re-exported from `locking_tx_datastore::mod`. - `datastore/datastore.rs`: replace the 4 `test_alter_table_event_flag_*` tests' inline is-event + st_event_table-row-presence duplication with `check_table_event_flag_altered` (was repeated 3x by the reviewer's count; actual count after deduplication drops to 5 call sites via the helper). Add `TxData` assertions on the successful commits proving the schema change materializes as an `st_event_table` insert/delete and does not touch the user-table row data. Swap `is_empty()` / `assert_matches!(&[])` for `assert_eq!(tx.pending_schema_changes(), [])`. - `core/update.rs`: cleaner v10 builder via `.with_event(is_event)` chain; extract `setup_events_table` returning `TableId`; move the row insert into a separate tx in the non-empty-fails test; replace `.any(|c| matches!(...))` with `assert_matches!([pat, ..])`; replace `.is_empty()` with `assert_eq!([])`; adopt the shared `assert_is_event_state` helper from `spacetimedb-datastore`. --- crates/core/src/db/update.rs | 84 ++++------- .../locking_tx_datastore/committed_state.rs | 1 + .../src/locking_tx_datastore/datastore.rs | 133 +++++------------- .../datastore/src/locking_tx_datastore/mod.rs | 2 + .../src/locking_tx_datastore/mut_tx.rs | 13 +- .../src/locking_tx_datastore/replay.rs | 1 + .../src/locking_tx_datastore/test_helpers.rs | 55 ++++++++ crates/schema/src/auto_migrate.rs | 107 +++++--------- 8 files changed, 174 insertions(+), 222 deletions(-) create mode 100644 crates/datastore/src/locking_tx_datastore/test_helpers.rs diff --git a/crates/core/src/db/update.rs b/crates/core/src/db/update.rs index 1207d9d73bc..251045f32ff 100644 --- a/crates/core/src/db/update.rs +++ b/crates/core/src/db/update.rs @@ -360,7 +360,8 @@ mod test { db::relational_db::tests_utils::{begin_mut_tx, insert, TestDB}, host::module_host::create_table_from_def, }; - use spacetimedb_datastore::locking_tx_datastore::state_view::StateView; + use pretty_assertions::assert_matches; + use spacetimedb_datastore::locking_tx_datastore::test_helpers::assert_is_event_state; use spacetimedb_datastore::locking_tx_datastore::PendingSchemaChange; use spacetimedb_lib::db::raw_def::v9::{btree, RawModuleDefV9Builder, TableAccess}; use spacetimedb_sats::{product, AlgebraicType, AlgebraicType::U64}; @@ -609,15 +610,30 @@ mod test { use spacetimedb_lib::db::raw_def::v10::RawModuleDefV10Builder; let mut builder = RawModuleDefV10Builder::new(); - let t = builder.build_table_with_new_type("events", [("id", U64)], true); - let t = if is_event { t.with_event(true) } else { t }; - t.finish(); + builder + .build_table_with_new_type("events", [("id", U64)], true) + .with_event(is_event) + .finish(); builder .finish() .try_into() .expect("should be a valid v10 module definition") } + /// Create a non-event `events` table from the schema of `single_event_table_module_v10(false)` + /// in a fresh tx, commit it, and return the `TableId`. Leaves the table empty. + fn setup_events_table(stdb: &TestDB, module: &ModuleDef) -> anyhow::Result { + let mut tx = begin_mut_tx(stdb); + for def in module.tables() { + create_table_from_def(stdb, &mut tx, module, def)?; + } + let table_id = stdb + .table_id_from_name_mut(&tx, "events")? + .expect("table should exist"); + stdb.commit_tx(tx)?; + Ok(table_id) + } + #[test] fn change_event_flag_empty_table_succeeds() -> anyhow::Result<()> { let auth_ctx = AuthCtx::for_testing(); @@ -625,25 +641,11 @@ mod test { let old = single_event_table_module_v10(false); let new = single_event_table_module_v10(true); + let table_id = setup_events_table(&stdb, &old)?; - // Create the non-event table. let mut tx = begin_mut_tx(&stdb); - for def in old.tables() { - create_table_from_def(&stdb, &mut tx, &old, def)?; - } - let table_id = stdb - .table_id_from_name_mut(&tx, "events")? - .expect("table should exist"); - assert!( - !tx.get_schema(table_id) - .map(|s| s.is_event) - .expect("schema should exist"), - "fresh non-event table should have `is_event=false`" - ); - stdb.commit_tx(tx)?; + assert_is_event_state(&tx, table_id, false); - // Apply the auto-migration flipping `is_event=true` on an empty table. - let mut tx = begin_mut_tx(&stdb); let plan = ponder_migrate(&old, &new)?; let res = update_database(&stdb, &mut tx, auth_ctx, plan, &TestLogger)?; @@ -651,18 +653,10 @@ mod test { matches!(res, UpdateResult::RequiresClientDisconnect), "flipping the `event` flag should disconnect clients" ); - assert!( - tx.get_schema(table_id) - .map(|s| s.is_event) - .expect("schema should exist"), - "live schema should reflect the new `is_event=true`" - ); - assert!( - tx.pending_schema_changes() - .iter() - .any(|c| matches!(c, PendingSchemaChange::TableAlterEventFlag(_, false))), - "flipping `is_event` should record a TableAlterEventFlag pending change: {:?}", - tx.pending_schema_changes() + assert_is_event_state(&tx, table_id, true); + assert_matches!( + tx.pending_schema_changes(), + [PendingSchemaChange::TableAlterEventFlag(t, false), ..] if *t == table_id ); Ok(()) } @@ -674,40 +668,24 @@ mod test { let old = single_event_table_module_v10(false); let new = single_event_table_module_v10(true); + let table_id = setup_events_table(&stdb, &old)?; - // Create the table, insert a row, then attempt to flip `is_event`. + // Insert a row in a separate tx so the pre-flip table state is committed. let mut tx = begin_mut_tx(&stdb); - for def in old.tables() { - create_table_from_def(&stdb, &mut tx, &old, def)?; - } - let table_id = stdb - .table_id_from_name_mut(&tx, "events")? - .expect("table should exist"); insert(&stdb, &mut tx, table_id, &product![42u64])?; stdb.commit_tx(tx)?; let mut tx = begin_mut_tx(&stdb); let plan = ponder_migrate(&old, &new)?; - let result = update_database(&stdb, &mut tx, auth_ctx, plan, &TestLogger); - let err = result + let err = update_database(&stdb, &mut tx, auth_ctx, plan, &TestLogger) .err() .expect("flipping `is_event` on a non-empty table should fail"); assert!( err.to_string().contains("contains data"), "error should mention that the table contains data, got: {err}" ); - // The schema should be untouched and there should be no pending changes to roll back. - assert!( - !tx.get_schema(table_id) - .map(|s| s.is_event) - .expect("schema should exist"), - "failed migration must leave the `is_event` flag unchanged" - ); - assert!( - tx.pending_schema_changes().is_empty(), - "failed migration should leave no pending schema changes: {:?}", - tx.pending_schema_changes() - ); + assert_is_event_state(&tx, table_id, false); + assert_eq!(tx.pending_schema_changes(), []); Ok(()) } } diff --git a/crates/datastore/src/locking_tx_datastore/committed_state.rs b/crates/datastore/src/locking_tx_datastore/committed_state.rs index 30cb597ed79..a048fd6173b 100644 --- a/crates/datastore/src/locking_tx_datastore/committed_state.rs +++ b/crates/datastore/src/locking_tx_datastore/committed_state.rs @@ -737,6 +737,7 @@ impl CommittedState { // A table's `is_event` flag was changed. Change back to the old one. TableAlterEventFlag(table_id, old_is_event) => { let table = self.tables.get_mut(&table_id)?; + assert_eq!(table.num_rows(), 0); table.with_mut_schema(|s| s.is_event = old_is_event); } // A table's primary key was changed. Change back to the old one. diff --git a/crates/datastore/src/locking_tx_datastore/datastore.rs b/crates/datastore/src/locking_tx_datastore/datastore.rs index 43207eb7aab..ce8e66624f7 100644 --- a/crates/datastore/src/locking_tx_datastore/datastore.rs +++ b/crates/datastore/src/locking_tx_datastore/datastore.rs @@ -979,7 +979,7 @@ pub(crate) mod tests { use crate::locking_tx_datastore::tx_state::PendingSchemaChange; use crate::system_tables::{ system_tables, StColumnRow, StConnectionCredentialsFields, StConstraintData, StConstraintFields, - StConstraintRow, StEventTableFields, StFields as _, StIndexAlgorithm, StIndexFields, StIndexRow, StRowLevelSecurityFields, + StConstraintRow, StEventTableFields, StIndexAlgorithm, StIndexFields, StIndexRow, StRowLevelSecurityFields, StScheduledFields, StSequenceFields, StSequenceRow, StTableRow, StVarFields, StViewArgFields, StViewFields, ST_CLIENT_ID, ST_CLIENT_NAME, ST_COLUMN_ACCESSOR_ID, ST_COLUMN_ACCESSOR_NAME, ST_COLUMN_ID, ST_COLUMN_NAME, ST_CONNECTION_CREDENTIALS_ID, ST_CONNECTION_CREDENTIALS_NAME, ST_CONSTRAINT_ID, ST_CONSTRAINT_NAME, @@ -3149,163 +3149,102 @@ pub(crate) mod tests { Ok(()) } - /// Returns whether `st_event_table` contains a row referencing `table_id`. - fn st_event_table_has_row(datastore: &Locking, tx: &MutTxId, table_id: TableId) -> bool { - datastore - .iter_by_col_eq_mut_tx( - tx, - ST_EVENT_TABLE_ID, - ColList::from(StEventTableFields::TableId.col_id()), - &table_id.into(), - ) - .expect("st_event_table lookup should succeed") - .next() - .is_some() - } - #[test] fn test_alter_table_event_flag_non_event_to_event() -> ResultTest<()> { - // Create a non-event table. + use crate::locking_tx_datastore::test_helpers::check_table_event_flag_altered; + let (datastore, tx, table_id) = setup_table()?; commit(&datastore, tx)?; - // Flip `is_event` from `false` to `true`. let mut tx = begin_mut_tx(&datastore); - assert_eq!( - tx.get_schema(table_id) - .map(|s| s.is_event) - .expect("schema should exist"), - false - ); - assert!( - !st_event_table_has_row(&datastore, &tx, table_id), - "fresh non-event table must not have a row in `st_event_table`" - ); + check_table_event_flag_altered(&datastore, &tx, table_id, false); tx.alter_table_event_flag(table_id, true)?; assert_matches!( tx.pending_schema_changes(), &[PendingSchemaChange::TableAlterEventFlag(t, false)] if t == table_id ); - commit(&datastore, tx)?; + check_table_event_flag_altered(&datastore, &tx, table_id, true); - // After commit, the schema should reflect the flipped flag - // and `st_event_table` should contain the row. - let tx = begin_mut_tx(&datastore); + let tx_data = commit(&datastore, tx)?; + // Flipping to event inserts one row into `st_event_table` + // and does not touch the user table's row data. assert_eq!( - tx.get_schema(table_id) - .map(|s| s.is_event) - .expect("schema should exist"), - true - ); - assert!( - st_event_table_has_row(&datastore, &tx, table_id), - "after flipping to event, `st_event_table` should have the row" + tx_data.inserts_for_table(ST_EVENT_TABLE_ID).map(<[_]>::len), + Some(1), ); + assert_eq!(tx_data.inserts_for_table(table_id), None); + assert_eq!(tx_data.deletes_for_table(table_id), None); + + let tx = begin_mut_tx(&datastore); + check_table_event_flag_altered(&datastore, &tx, table_id, true); Ok(()) } #[test] fn test_alter_table_event_flag_event_to_non_event() -> ResultTest<()> { - // Create an event table. + use crate::locking_tx_datastore::test_helpers::check_table_event_flag_altered; + let (datastore, tx, table_id) = setup_event_table()?; commit(&datastore, tx)?; - // Sanity check: `st_event_table` should have the row. let mut tx = begin_mut_tx(&datastore); - assert_eq!( - tx.get_schema(table_id) - .map(|s| s.is_event) - .expect("schema should exist"), - true - ); - assert!( - st_event_table_has_row(&datastore, &tx, table_id), - "event table should have a row in `st_event_table`" - ); + check_table_event_flag_altered(&datastore, &tx, table_id, true); - // Flip `is_event` from `true` to `false`. tx.alter_table_event_flag(table_id, false)?; assert_matches!( tx.pending_schema_changes(), &[PendingSchemaChange::TableAlterEventFlag(t, true)] if t == table_id ); - commit(&datastore, tx)?; + check_table_event_flag_altered(&datastore, &tx, table_id, false); - // After commit, the schema should reflect the flipped flag - // and `st_event_table` should NOT contain the row. - let tx = begin_mut_tx(&datastore); + let tx_data = commit(&datastore, tx)?; + // Flipping away from event deletes one row from `st_event_table` + // and does not touch the user table's row data. assert_eq!( - tx.get_schema(table_id) - .map(|s| s.is_event) - .expect("schema should exist"), - false - ); - assert!( - !st_event_table_has_row(&datastore, &tx, table_id), - "after flipping to non-event, `st_event_table` should not have the row" + tx_data.deletes_for_table(ST_EVENT_TABLE_ID).map(<[_]>::len), + Some(1), ); + assert_eq!(tx_data.inserts_for_table(table_id), None); + assert_eq!(tx_data.deletes_for_table(table_id), None); + + let tx = begin_mut_tx(&datastore); + check_table_event_flag_altered(&datastore, &tx, table_id, false); Ok(()) } #[test] fn test_alter_table_event_flag_rollback_reverts_live_state_and_st_event_table() -> ResultTest<()> { - // Create a non-event table. + use crate::locking_tx_datastore::test_helpers::check_table_event_flag_altered; + let (datastore, tx, table_id) = setup_table()?; commit(&datastore, tx)?; - // Start a new tx, flip, check pending change, then rollback. let mut tx = begin_mut_tx(&datastore); - assert!(!st_event_table_has_row(&datastore, &tx, table_id)); + check_table_event_flag_altered(&datastore, &tx, table_id, false); tx.alter_table_event_flag(table_id, true)?; assert_matches!( tx.pending_schema_changes(), &[PendingSchemaChange::TableAlterEventFlag(t, false)] if t == table_id ); - // The in-tx view must reflect the flip. - assert_eq!( - tx.get_schema(table_id) - .map(|s| s.is_event) - .expect("schema should exist"), - true - ); - assert!( - st_event_table_has_row(&datastore, &tx, table_id), - "after flipping within the tx, `st_event_table` should have the row" - ); + check_table_event_flag_altered(&datastore, &tx, table_id, true); let _ = datastore.rollback_mut_tx(tx); - // After rollback, the schema and `st_event_table` should be back to pre-state. let tx = begin_mut_tx(&datastore); - assert!( - tx.pending_schema_changes().is_empty(), - "rollback should clear pending schema changes" - ); - assert_eq!( - tx.get_schema(table_id) - .map(|s| s.is_event) - .expect("schema should exist"), - false, - "rollback should revert `is_event` to its pre-tx value" - ); - assert!( - !st_event_table_has_row(&datastore, &tx, table_id), - "rollback should revert the `st_event_table` row" - ); + assert_eq!(tx.pending_schema_changes(), []); + check_table_event_flag_altered(&datastore, &tx, table_id, false); Ok(()) } #[test] fn test_alter_table_event_flag_idempotent_no_pending_change() -> ResultTest<()> { - // Create a non-event table. let (datastore, tx, table_id) = setup_table()?; commit(&datastore, tx)?; - // Flipping to the same value must be a no-op with no pending change. let mut tx = begin_mut_tx(&datastore); tx.alter_table_event_flag(table_id, false)?; - assert_matches!(tx.pending_schema_changes(), &[]); + assert_eq!(tx.pending_schema_changes(), []); Ok(()) } diff --git a/crates/datastore/src/locking_tx_datastore/mod.rs b/crates/datastore/src/locking_tx_datastore/mod.rs index 8f77b462bdd..aac824bc1aa 100644 --- a/crates/datastore/src/locking_tx_datastore/mod.rs +++ b/crates/datastore/src/locking_tx_datastore/mod.rs @@ -15,6 +15,8 @@ pub use tx::{NumDistinctValues, TxId}; mod tx_state; #[cfg(any(test, feature = "test"))] pub use tx_state::PendingSchemaChange; +#[cfg(any(test, feature = "test"))] +pub mod test_helpers; use parking_lot::{ lock_api::{ArcMutexGuard, ArcRwLockReadGuard, ArcRwLockWriteGuard}, diff --git a/crates/datastore/src/locking_tx_datastore/mut_tx.rs b/crates/datastore/src/locking_tx_datastore/mut_tx.rs index 9b3813b6b9f..4c32c9c92f1 100644 --- a/crates/datastore/src/locking_tx_datastore/mut_tx.rs +++ b/crates/datastore/src/locking_tx_datastore/mut_tx.rs @@ -691,8 +691,7 @@ impl MutTxId { // Insert into st_event_table if this is an event table. if is_event { - let row = StEventTableRow { table_id }; - self.insert_via_serialize_bsatn(ST_EVENT_TABLE_ID, &row)?; + self.insert_st_event_table_row(table_id)?; } // Create the indexes for the table. @@ -1099,8 +1098,7 @@ impl MutTxId { // Update `st_event_table`. if is_event { - let row = StEventTableRow { table_id }; - self.insert_via_serialize_bsatn(ST_EVENT_TABLE_ID, &row)?; + self.insert_st_event_table_row(table_id)?; } else { self.delete_st_event_table_row(table_id)?; } @@ -1111,6 +1109,13 @@ impl MutTxId { Ok(()) } + /// Inserts a row into `st_event_table` marking `table_id` as an event table. + fn insert_st_event_table_row(&mut self, table_id: TableId) -> Result<()> { + let row = StEventTableRow { table_id }; + self.insert_via_serialize_bsatn(ST_EVENT_TABLE_ID, &row)?; + Ok(()) + } + /// Drops the row in `st_event_table` for this `table_id`. fn delete_st_event_table_row(&mut self, table_id: TableId) -> Result<()> { self.delete_col_eq( diff --git a/crates/datastore/src/locking_tx_datastore/replay.rs b/crates/datastore/src/locking_tx_datastore/replay.rs index 47a6194607c..2f9a4e9cff8 100644 --- a/crates/datastore/src/locking_tx_datastore/replay.rs +++ b/crates/datastore/src/locking_tx_datastore/replay.rs @@ -822,6 +822,7 @@ impl<'cs> ReplayCommittedState<'cs> { // If we haven't yet, then `self.get_table_and_blob_store_or_create` will see the correct schema // (via `schema_for_table_raw`'s live `st_event_table` lookup) when it eventually runs. if let Ok((table, ..)) = self.get_table_and_blob_store_mut(table_id) { + assert_eq!(table.num_rows(), 0); table.with_mut_schema(|schema| { schema.is_event = is_event; }); diff --git a/crates/datastore/src/locking_tx_datastore/test_helpers.rs b/crates/datastore/src/locking_tx_datastore/test_helpers.rs new file mode 100644 index 00000000000..102354e0446 --- /dev/null +++ b/crates/datastore/src/locking_tx_datastore/test_helpers.rs @@ -0,0 +1,55 @@ +//! Test-only helpers shared between the datastore's internal tests and +//! downstream-crate tests (e.g. `spacetimedb-core`'s `update.rs`). +//! +//! These are gated by `#[cfg(any(test, feature = "test"))]` and re-exported +//! from `locking_tx_datastore::mod` so they are reachable from other crates +//! that enable the `test` feature. + +use super::datastore::Locking; +use super::state_view::StateView as _; +use super::MutTxId; +use crate::system_tables::{StEventTableFields, StFields as _, ST_EVENT_TABLE_ID}; +use crate::traits::MutTxDatastore as _; +use spacetimedb_primitives::{ColList, TableId}; + +/// Asserts that the live schema's `is_event` flag for `table_id` equals `expected`. +pub fn assert_is_event_state(tx: &MutTxId, table_id: TableId, expected: bool) { + let actual = tx + .get_schema(table_id) + .map(|s| s.is_event) + .expect("schema should exist"); + assert_eq!( + actual, expected, + "expected table {table_id:?} is_event={expected}" + ); +} + +/// Returns whether `st_event_table` contains a row referencing `table_id`. +pub fn st_event_table_has_row(datastore: &Locking, tx: &MutTxId, table_id: TableId) -> bool { + datastore + .iter_by_col_eq_mut_tx( + tx, + ST_EVENT_TABLE_ID, + ColList::from(StEventTableFields::TableId.col_id()), + &table_id.into(), + ) + .expect("st_event_table lookup should succeed") + .next() + .is_some() +} + +/// Asserts that the live schema's `is_event` equals `expected` AND that +/// `st_event_table` has a row referencing `table_id` iff `expected` is true. +pub fn check_table_event_flag_altered( + datastore: &Locking, + tx: &MutTxId, + table_id: TableId, + expected: bool, +) { + assert_is_event_state(tx, table_id, expected); + assert_eq!( + st_event_table_has_row(datastore, tx, table_id), + expected, + "st_event_table row presence should match is_event={expected}" + ); +} diff --git a/crates/schema/src/auto_migrate.rs b/crates/schema/src/auto_migrate.rs index 6fce9345600..af1585e082b 100644 --- a/crates/schema/src/auto_migrate.rs +++ b/crates/schema/src/auto_migrate.rs @@ -2393,82 +2393,53 @@ mod tests { } #[test] - fn test_change_event_flag_produces_step_non_to_event() { - // non-event → event - let old = create_v10_module_def(|builder| { - builder - .build_table_with_new_type("events", ProductType::from([("id", AlgebraicType::U64)]), true) - .finish(); - }); - let new = create_v10_module_def(|builder| { - builder - .build_table_with_new_type("events", ProductType::from([("id", AlgebraicType::U64)]), true) - .with_event(true) - .finish(); - }); - - let events_key = expect_identifier("events"); - let plan = ponder_auto_migrate(&old, &new).expect("toggling `is_event` on an empty table should succeed"); - assert_eq!( - plan.steps, - &[ - AutoMigrateStep::ChangeEventFlag(&events_key), - AutoMigrateStep::DisconnectAllUsers, - ], - ); - assert!(plan.disconnects_all_users(), "{plan:#?}"); - } - - #[test] - fn test_change_event_flag_produces_step_event_to_non() { - // event → non-event - let old = create_v10_module_def(|builder| { - builder - .build_table_with_new_type("events", ProductType::from([("id", AlgebraicType::U64)]), true) - .with_event(true) - .finish(); - }); - let new = create_v10_module_def(|builder| { - builder - .build_table_with_new_type("events", ProductType::from([("id", AlgebraicType::U64)]), true) - .finish(); - }); - - let events_key = expect_identifier("events"); - let plan = ponder_auto_migrate(&old, &new).expect("toggling `is_event` on an empty table should succeed"); - assert_eq!( - plan.steps, - &[ - AutoMigrateStep::ChangeEventFlag(&events_key), - AutoMigrateStep::DisconnectAllUsers, - ], - ); - assert!(plan.disconnects_all_users(), "{plan:#?}"); + fn test_change_event_flag_produces_step() { + let build = |is_event: bool| { + create_v10_module_def(|builder| { + builder + .build_table_with_new_type("events", ProductType::from([("id", AlgebraicType::U64)]), true) + .with_event(is_event) + .finish(); + }) + }; + let assert_flip = |old_is_event: bool, new_is_event: bool| { + let old = build(old_is_event); + let new = build(new_is_event); + let events_key = expect_identifier("events"); + let plan = ponder_auto_migrate(&old, &new) + .expect("toggling `is_event` on an empty table should succeed"); + assert_eq!( + plan.steps, + &[ + AutoMigrateStep::ChangeEventFlag(&events_key), + AutoMigrateStep::DisconnectAllUsers, + ], + ); + assert!(plan.disconnects_all_users(), "{plan:#?}"); + }; + assert_flip(false, true); + assert_flip(true, false); } #[test] fn test_change_event_flag_does_not_produce_orphan_sub_object_steps() { // Flipping `is_event` must not trigger spurious index/constraint/sequence diff steps // for a table whose only change is the `is_event` annotation. - let old = create_v10_module_def(|builder| { - builder - .build_table_with_new_type("events", ProductType::from([("id", AlgebraicType::U64)]), true) - .with_unique_constraint(0) - .with_index(btree(0), "events_id_idx", "events_id_idx") - .with_primary_key(0) - .finish(); - }); - let new = create_v10_module_def(|builder| { - builder - .build_table_with_new_type("events", ProductType::from([("id", AlgebraicType::U64)]), true) - .with_unique_constraint(0) - .with_index(btree(0), "events_id_idx", "events_id_idx") - .with_primary_key(0) - .with_event(true) - .finish(); - }); + let build = |is_event: bool| { + create_v10_module_def(|builder| { + builder + .build_table_with_new_type("events", ProductType::from([("id", AlgebraicType::U64)]), true) + .with_unique_constraint(0) + .with_index(btree(0), "events_id_idx", "events_id_idx") + .with_primary_key(0) + .with_event(is_event) + .finish(); + }) + }; let events_key = expect_identifier("events"); + let old = build(false); + let new = build(true); let plan = ponder_auto_migrate(&old, &new).expect("toggling `is_event` on an empty table should succeed"); assert_eq!( plan.steps, From fb321ac85b9af390ce9df56202673e381c71c0bf Mon Sep 17 00:00:00 2001 From: Ludv1g Date: Fri, 24 Apr 2026 22:12:25 +0200 Subject: [PATCH 3/3] refactor(event-flag): apply Centril 2026-04-24 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third round of review fixes. All 9 actionable comments addressed, no incidental cleanup. - `test_helpers.rs`: - `st_event_table_has_row`: drop the `ColList::from(...).col_id()` wrapping and pass `StEventTableFields::TableId` directly (the `iter_by_col_eq_mut_tx` `cols: impl Into` + blanket `impl> From for ColList` handle the conversion). Matches the idiom in `system_tables.rs` (`with_primary_key`, `with_unique_constraint`, `btree(...)`). - Rename: the previous bundled `check_table_event_flag_altered` (schema + `st_event_table` row-presence check) is replaced by a version that asserts `tx.pending_schema_changes()` contains exactly one `TableAlterEventFlag(table_id, !state)` entry — matching the repeated assertion pattern that Centril asked to factor out in round 2. - `datastore.rs` tests: - Hoist the `test_helpers::{assert_is_event_state, check_table_event_flag_altered, st_event_table_has_row}` import to the module level (was duplicated inside each of the 4 tests). - Restore the per-step `// Create a non-event table.` / `// Flip is_event from false to true.` / `// After rollback...` narrative comments and assertion messages that were lost in the previous refactor. - Replace the `inserts_for_table(ST_EVENT_TABLE_ID).map(<[_]>::len) == Some(1)` length-only check (and its `deletes_for_table` counterpart) with an exact row-content assertion against `ProductValue::from(StEventTableRow { table_id })`. - `core/db/update.rs`: - Swap the inline `assert_matches!(tx.pending_schema_changes(), [PendingSchemaChange::TableAlterEventFlag(t, false), ..] if *t == table_id)` for a call to the shared `check_table_event_flag_altered` helper (addresses both the "assert_eq is simpler" suggestion and the round-2 request that the helper live in `crates/datastore` and be used from `update.rs`). Validation: - `cargo test -p spacetimedb-datastore --features test --lib test_alter_table_event_flag` — 4/4 pass - `cargo test -p spacetimedb-core --lib change_event_flag` — 2/2 pass - `cargo clippy -p spacetimedb-datastore --features test --tests` — clean Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/core/src/db/update.rs | 9 +- .../src/locking_tx_datastore/datastore.rs | 96 +++++++++++++------ .../src/locking_tx_datastore/test_helpers.rs | 35 +++---- 3 files changed, 81 insertions(+), 59 deletions(-) diff --git a/crates/core/src/db/update.rs b/crates/core/src/db/update.rs index 251045f32ff..31c5b496b7c 100644 --- a/crates/core/src/db/update.rs +++ b/crates/core/src/db/update.rs @@ -361,7 +361,9 @@ mod test { host::module_host::create_table_from_def, }; use pretty_assertions::assert_matches; - use spacetimedb_datastore::locking_tx_datastore::test_helpers::assert_is_event_state; + use spacetimedb_datastore::locking_tx_datastore::test_helpers::{ + assert_is_event_state, check_table_event_flag_altered, + }; use spacetimedb_datastore::locking_tx_datastore::PendingSchemaChange; use spacetimedb_lib::db::raw_def::v9::{btree, RawModuleDefV9Builder, TableAccess}; use spacetimedb_sats::{product, AlgebraicType, AlgebraicType::U64}; @@ -654,10 +656,7 @@ mod test { "flipping the `event` flag should disconnect clients" ); assert_is_event_state(&tx, table_id, true); - assert_matches!( - tx.pending_schema_changes(), - [PendingSchemaChange::TableAlterEventFlag(t, false), ..] if *t == table_id - ); + check_table_event_flag_altered(&tx, table_id, true); Ok(()) } diff --git a/crates/datastore/src/locking_tx_datastore/datastore.rs b/crates/datastore/src/locking_tx_datastore/datastore.rs index ce8e66624f7..e6fd87fe790 100644 --- a/crates/datastore/src/locking_tx_datastore/datastore.rs +++ b/crates/datastore/src/locking_tx_datastore/datastore.rs @@ -976,11 +976,15 @@ fn metadata_from_row(row: RowRef<'_>) -> Result { pub(crate) mod tests { use super::*; use crate::error::IndexError; + use crate::locking_tx_datastore::test_helpers::{ + assert_is_event_state, check_table_event_flag_altered, st_event_table_has_row, + }; use crate::locking_tx_datastore::tx_state::PendingSchemaChange; use crate::system_tables::{ system_tables, StColumnRow, StConnectionCredentialsFields, StConstraintData, StConstraintFields, - StConstraintRow, StEventTableFields, StIndexAlgorithm, StIndexFields, StIndexRow, StRowLevelSecurityFields, - StScheduledFields, StSequenceFields, StSequenceRow, StTableRow, StVarFields, StViewArgFields, StViewFields, + StConstraintRow, StEventTableFields, StEventTableRow, StIndexAlgorithm, StIndexFields, StIndexRow, + StRowLevelSecurityFields, StScheduledFields, StSequenceFields, StSequenceRow, StTableRow, StVarFields, + StViewArgFields, StViewFields, ST_CLIENT_ID, ST_CLIENT_NAME, ST_COLUMN_ACCESSOR_ID, ST_COLUMN_ACCESSOR_NAME, ST_COLUMN_ID, ST_COLUMN_NAME, ST_CONNECTION_CREDENTIALS_ID, ST_CONNECTION_CREDENTIALS_NAME, ST_CONSTRAINT_ID, ST_CONSTRAINT_NAME, ST_EVENT_TABLE_ID, ST_EVENT_TABLE_NAME, ST_INDEX_ACCESSOR_ID, ST_INDEX_ACCESSOR_NAME, ST_INDEX_ID, @@ -3151,89 +3155,121 @@ pub(crate) mod tests { #[test] fn test_alter_table_event_flag_non_event_to_event() -> ResultTest<()> { - use crate::locking_tx_datastore::test_helpers::check_table_event_flag_altered; - + // Create a non-event table. let (datastore, tx, table_id) = setup_table()?; commit(&datastore, tx)?; + // Flip `is_event` from `false` to `true`. let mut tx = begin_mut_tx(&datastore); - check_table_event_flag_altered(&datastore, &tx, table_id, false); + assert_is_event_state(&tx, table_id, false); + assert!( + !st_event_table_has_row(&datastore, &tx, table_id), + "fresh non-event table must not have a row in `st_event_table`" + ); tx.alter_table_event_flag(table_id, true)?; - assert_matches!( - tx.pending_schema_changes(), - &[PendingSchemaChange::TableAlterEventFlag(t, false)] if t == table_id + check_table_event_flag_altered(&tx, table_id, true); + assert_is_event_state(&tx, table_id, true); + assert!( + st_event_table_has_row(&datastore, &tx, table_id), + "after flipping to event, `st_event_table` should have the row" ); - check_table_event_flag_altered(&datastore, &tx, table_id, true); let tx_data = commit(&datastore, tx)?; // Flipping to event inserts one row into `st_event_table` // and does not touch the user table's row data. + let expected_row = ProductValue::from(StEventTableRow { table_id }); assert_eq!( - tx_data.inserts_for_table(ST_EVENT_TABLE_ID).map(<[_]>::len), - Some(1), + tx_data.inserts_for_table(ST_EVENT_TABLE_ID), + Some(&[expected_row][..]), ); assert_eq!(tx_data.inserts_for_table(table_id), None); assert_eq!(tx_data.deletes_for_table(table_id), None); + // After commit, the schema should reflect the flipped flag + // and `st_event_table` should contain the row. let tx = begin_mut_tx(&datastore); - check_table_event_flag_altered(&datastore, &tx, table_id, true); + assert_is_event_state(&tx, table_id, true); + assert!( + st_event_table_has_row(&datastore, &tx, table_id), + "after commit, `st_event_table` should have the row" + ); Ok(()) } #[test] fn test_alter_table_event_flag_event_to_non_event() -> ResultTest<()> { - use crate::locking_tx_datastore::test_helpers::check_table_event_flag_altered; - + // Create an event table. let (datastore, tx, table_id) = setup_event_table()?; commit(&datastore, tx)?; + // Sanity check: `st_event_table` should have the row. let mut tx = begin_mut_tx(&datastore); - check_table_event_flag_altered(&datastore, &tx, table_id, true); + assert_is_event_state(&tx, table_id, true); + assert!( + st_event_table_has_row(&datastore, &tx, table_id), + "event table should have a row in `st_event_table`" + ); + // Flip `is_event` from `true` to `false`. tx.alter_table_event_flag(table_id, false)?; - assert_matches!( - tx.pending_schema_changes(), - &[PendingSchemaChange::TableAlterEventFlag(t, true)] if t == table_id + check_table_event_flag_altered(&tx, table_id, false); + assert_is_event_state(&tx, table_id, false); + assert!( + !st_event_table_has_row(&datastore, &tx, table_id), + "after flipping to non-event, `st_event_table` should not have the row" ); - check_table_event_flag_altered(&datastore, &tx, table_id, false); let tx_data = commit(&datastore, tx)?; // Flipping away from event deletes one row from `st_event_table` // and does not touch the user table's row data. + let expected_row = ProductValue::from(StEventTableRow { table_id }); assert_eq!( - tx_data.deletes_for_table(ST_EVENT_TABLE_ID).map(<[_]>::len), - Some(1), + tx_data.deletes_for_table(ST_EVENT_TABLE_ID), + Some(&[expected_row][..]), ); assert_eq!(tx_data.inserts_for_table(table_id), None); assert_eq!(tx_data.deletes_for_table(table_id), None); + // After commit, the schema should reflect the flipped flag + // and `st_event_table` should NOT contain the row. let tx = begin_mut_tx(&datastore); - check_table_event_flag_altered(&datastore, &tx, table_id, false); + assert_is_event_state(&tx, table_id, false); + assert!( + !st_event_table_has_row(&datastore, &tx, table_id), + "after commit, `st_event_table` should not have the row" + ); Ok(()) } #[test] fn test_alter_table_event_flag_rollback_reverts_live_state_and_st_event_table() -> ResultTest<()> { - use crate::locking_tx_datastore::test_helpers::check_table_event_flag_altered; - + // Create a non-event table. let (datastore, tx, table_id) = setup_table()?; commit(&datastore, tx)?; + // Start a new tx, flip, check pending change, then rollback. let mut tx = begin_mut_tx(&datastore); - check_table_event_flag_altered(&datastore, &tx, table_id, false); + assert!(!st_event_table_has_row(&datastore, &tx, table_id)); tx.alter_table_event_flag(table_id, true)?; - assert_matches!( - tx.pending_schema_changes(), - &[PendingSchemaChange::TableAlterEventFlag(t, false)] if t == table_id + check_table_event_flag_altered(&tx, table_id, true); + // The in-tx view must reflect the flip. + assert_is_event_state(&tx, table_id, true); + assert!( + st_event_table_has_row(&datastore, &tx, table_id), + "after flipping within the tx, `st_event_table` should have the row" ); - check_table_event_flag_altered(&datastore, &tx, table_id, true); let _ = datastore.rollback_mut_tx(tx); + // After rollback, the schema and `st_event_table` should be back to pre-state. let tx = begin_mut_tx(&datastore); assert_eq!(tx.pending_schema_changes(), []); - check_table_event_flag_altered(&datastore, &tx, table_id, false); + assert_is_event_state(&tx, table_id, false); + assert!( + !st_event_table_has_row(&datastore, &tx, table_id), + "rollback should revert the `st_event_table` row" + ); Ok(()) } diff --git a/crates/datastore/src/locking_tx_datastore/test_helpers.rs b/crates/datastore/src/locking_tx_datastore/test_helpers.rs index 102354e0446..99e7fb1dd41 100644 --- a/crates/datastore/src/locking_tx_datastore/test_helpers.rs +++ b/crates/datastore/src/locking_tx_datastore/test_helpers.rs @@ -7,10 +7,11 @@ use super::datastore::Locking; use super::state_view::StateView as _; +use super::tx_state::PendingSchemaChange; use super::MutTxId; -use crate::system_tables::{StEventTableFields, StFields as _, ST_EVENT_TABLE_ID}; +use crate::system_tables::{StEventTableFields, ST_EVENT_TABLE_ID}; use crate::traits::MutTxDatastore as _; -use spacetimedb_primitives::{ColList, TableId}; +use spacetimedb_primitives::TableId; /// Asserts that the live schema's `is_event` flag for `table_id` equals `expected`. pub fn assert_is_event_state(tx: &MutTxId, table_id: TableId, expected: bool) { @@ -18,38 +19,24 @@ pub fn assert_is_event_state(tx: &MutTxId, table_id: TableId, expected: bool) { .get_schema(table_id) .map(|s| s.is_event) .expect("schema should exist"); - assert_eq!( - actual, expected, - "expected table {table_id:?} is_event={expected}" - ); + assert_eq!(actual, expected, "expected table {table_id:?} is_event={expected}"); } /// Returns whether `st_event_table` contains a row referencing `table_id`. pub fn st_event_table_has_row(datastore: &Locking, tx: &MutTxId, table_id: TableId) -> bool { datastore - .iter_by_col_eq_mut_tx( - tx, - ST_EVENT_TABLE_ID, - ColList::from(StEventTableFields::TableId.col_id()), - &table_id.into(), - ) + .iter_by_col_eq_mut_tx(tx, ST_EVENT_TABLE_ID, StEventTableFields::TableId, &table_id.into()) .expect("st_event_table lookup should succeed") .next() .is_some() } -/// Asserts that the live schema's `is_event` equals `expected` AND that -/// `st_event_table` has a row referencing `table_id` iff `expected` is true. -pub fn check_table_event_flag_altered( - datastore: &Locking, - tx: &MutTxId, - table_id: TableId, - expected: bool, -) { - assert_is_event_state(tx, table_id, expected); +/// Asserts that `tx.pending_schema_changes()` contains exactly one +/// `TableAlterEventFlag` change for `table_id` recording the old value +/// (i.e. the value just before we altered to `state`). +pub fn check_table_event_flag_altered(tx: &MutTxId, table_id: TableId, state: bool) { assert_eq!( - st_event_table_has_row(datastore, tx, table_id), - expected, - "st_event_table row presence should match is_event={expected}" + tx.pending_schema_changes(), + [PendingSchemaChange::TableAlterEventFlag(table_id, !state)] ); }