From 4fb542f332edb87e8d9c91b115d7b9ce0940bca5 Mon Sep 17 00:00:00 2001 From: Alessandro Asoni Date: Mon, 1 Jun 2026 15:48:22 +0200 Subject: [PATCH] Migrations: handle mounted submodule tables, views, and indexes --- crates/core/src/db/update.rs | 185 +- crates/schema/src/auto_migrate.rs | 1525 ++++++++++++----- crates/schema/src/auto_migrate/formatter.rs | 235 ++- .../src/auto_migrate/termcolor_formatter.rs | 3 +- crates/schema/src/identifier.rs | 72 + crates/schema/src/schema.rs | 23 +- ..._tests__updated pretty print no color.snap | 4 +- ..._migrate__tests__updated pretty print.snap | 4 +- 8 files changed, 1381 insertions(+), 670 deletions(-) diff --git a/crates/core/src/db/update.rs b/crates/core/src/db/update.rs index f9ca4c110d9..fa70bed380d 100644 --- a/crates/core/src/db/update.rs +++ b/crates/core/src/db/update.rs @@ -5,10 +5,9 @@ use spacetimedb_datastore::locking_tx_datastore::MutTxId; use spacetimedb_lib::db::auth::StTableType; use spacetimedb_lib::identity::AuthCtx; use spacetimedb_lib::AlgebraicValue; -use spacetimedb_primitives::{ColSet, TableId}; +use spacetimedb_primitives::ColSet; use spacetimedb_schema::auto_migrate::{AutoMigratePlan, ManualMigratePlan, MigratePlan}; -use spacetimedb_schema::def::{TableDef, ViewDef}; -use spacetimedb_schema::schema::{column_schemas_from_defs, IndexSchema, Schema, SequenceSchema, TableSchema}; +use spacetimedb_schema::schema::{column_schemas_from_defs, IndexSchema, Schema, SequenceSchema}; /// The logger used for by [`update_database`] and friends. pub trait UpdateLogger { @@ -51,15 +50,29 @@ pub fn update_database( // TODO: consider using `ErrorStream` here. let old_module_def = plan.old_def(); + + // Build a full-name → (owning_def, table_def) map covering root and all mounted tables. + // Mounted tables are stored in the DB with prefixed names like "lib.library_procedure_timer", + // but `ModuleDef::table()` only searches the root level. `all_tables_with_prefix()` returns + // the owning submodule alongside each def, which is also needed so that `check_compatible` + // resolves column type refs against the correct (sub)module typespace. + let old_tables_by_name: std::collections::HashMap = old_module_def + .all_tables_with_prefix() + .into_iter() + .map(|(prefix, owning_def, table_def)| { + (format!("{}{}", prefix, &table_def.name[..]), (owning_def, table_def)) + }) + .collect(); + for table in existing_tables .iter() .filter(|table| table.table_type != StTableType::System && !table.is_view()) { - let old_def = old_module_def - .table(&table.table_name[..]) + let (owning_def, old_def) = old_tables_by_name + .get(table.table_name.as_ref()) .ok_or_else(|| anyhow::anyhow!("table {} not found in old_module_def", table.table_name))?; - table.check_compatible(old_module_def, old_def)?; + table.check_compatible(owning_def, old_def)?; } match plan { @@ -102,9 +115,12 @@ fn auto_migrate_database( for precheck in plan.prechecks { match precheck { spacetimedb_schema::auto_migrate::AutoMigratePrecheck::CheckAddSequenceRangeValid(sequence_name) => { - let table_def = plan.new.stored_in_table_def(sequence_name).unwrap(); - let sequence_def = &table_def.sequences[sequence_name]; - let table_id = stdb.table_id_from_name_mut(tx, &table_def.name)?.unwrap(); + let (prefix, _owning_def, table_def, sequence_def) = plan + .new + .find_sequence_by_full_name(&*sequence_name) + .ok_or_else(|| anyhow::anyhow!("Precheck: sequence `{sequence_name}` not found in new module def"))?; + let table_full_name = format!("{}{}", prefix, &*table_def.accessor_name); + let table_id = stdb.table_id_from_name_mut(tx, &table_full_name)?.unwrap(); let ty = table_def .get_column(sequence_def.column) @@ -142,7 +158,7 @@ fn auto_migrate_database( for step in plan.steps { match step { spacetimedb_schema::auto_migrate::AutoMigrateStep::RemoveTable(table_name) => { - let table_id = stdb.table_id_from_name_mut(tx, table_name)?.unwrap(); + let table_id = stdb.table_id_from_name_mut(tx, &*table_name)?.unwrap(); if stdb.table_row_count_mut(tx, table_id).unwrap_or(0) > 0 { anyhow::bail!( @@ -155,22 +171,22 @@ fn auto_migrate_database( stdb.drop_table(tx, table_id)?; } spacetimedb_schema::auto_migrate::AutoMigrateStep::AddTable(table_name) => { - let table_def: &TableDef = plan.new.expect_lookup(table_name); - - // Recursively sets IDs to 0. - // They will be initialized by the database when the table is created. - let table_schema = TableSchema::from_module_def(plan.new, table_def, (), TableId::SENTINEL); - + let (prefix, owning_def, table_def) = plan + .new + .find_table_by_full_name(&*table_name) + .ok_or_else(|| anyhow::anyhow!("AddTable: table `{table_name}` not found in new module def"))?; log!(logger, "Creating table `{table_name}`"); - - stdb.create_table(tx, table_schema)?; + crate::host::module_host::create_table_from_def_with_prefix(stdb, tx, owning_def, table_def, &prefix)?; } spacetimedb_schema::auto_migrate::AutoMigrateStep::AddView(view_name) => { - let view_def: &ViewDef = plan.new.expect_lookup(view_name); - stdb.create_view(tx, plan.new, view_def)?; + let (prefix, owning_def, view_def) = plan + .new + .find_view_by_full_name(&*view_name) + .ok_or_else(|| anyhow::anyhow!("AddView: view `{view_name}` not found in new module def"))?; + crate::host::module_host::create_table_from_view_def_with_prefix(stdb, tx, owning_def, view_def, &prefix)?; } spacetimedb_schema::auto_migrate::AutoMigrateStep::RemoveView(view_name) => { - let view_id = stdb.view_id_from_name_mut(tx, view_name)?.unwrap(); + let view_id = stdb.view_id_from_name_mut(tx, &*view_name)?.unwrap(); stdb.drop_view(tx, view_id)?; } spacetimedb_schema::auto_migrate::AutoMigrateStep::UpdateView(_) => { @@ -181,9 +197,12 @@ fn auto_migrate_database( } } spacetimedb_schema::auto_migrate::AutoMigrateStep::AddIndex(index_name) => { - let table_def = plan.new.stored_in_table_def(index_name).unwrap(); - let index_def = table_def.indexes.get(index_name).unwrap(); - let table_id = stdb.table_id_from_name_mut(tx, &table_def.name)?.unwrap(); + let (prefix, owning_def, table_def, index_def) = plan + .new + .find_index_by_full_name(&*index_name) + .ok_or_else(|| anyhow::anyhow!("AddIndex: index `{index_name}` not found in new module def"))?; + let table_full_name = format!("{}{}", prefix, &*table_def.accessor_name); + let table_id = stdb.table_id_from_name_mut(tx, &table_full_name)?.unwrap(); let index_cols = ColSet::from(index_def.algorithm.columns()); @@ -193,99 +212,105 @@ fn auto_migrate_database( .filter_map(|(_, c)| c.data.unique_columns()) .any(|unique_cols| unique_cols == &index_cols); - log!(logger, "Creating index `{}` on table `{}`", index_name, table_def.name); + log!(logger, "Creating index `{index_name}` on table `{table_full_name}`"); - let index_schema = IndexSchema::from_module_def(plan.new, index_def, table_id, 0.into()); + let index_schema = IndexSchema::from_module_def(owning_def, index_def, table_id, 0.into()); stdb.create_index(tx, index_schema, is_unique)?; } spacetimedb_schema::auto_migrate::AutoMigrateStep::RemoveIndex(index_name) => { - let table_def = plan.old.stored_in_table_def(index_name).unwrap(); - - let table_id = stdb.table_id_from_name_mut(tx, &table_def.name)?.unwrap(); + let (prefix, _owning_def, table_def, _index_def) = plan + .old + .find_index_by_full_name(&*index_name) + .ok_or_else(|| anyhow::anyhow!("RemoveIndex: index `{index_name}` not found in old module def"))?; + let table_full_name = format!("{}{}", prefix, &*table_def.accessor_name); + let table_id = stdb.table_id_from_name_mut(tx, &table_full_name)?.unwrap(); let table_schema = stdb.schema_for_table_mut(tx, table_id)?; let index_schema = table_schema .indexes .iter() - .find(|index| index.index_name[..] == index_name[..]) + .find(|index| index.index_name[..] == *index_name) .unwrap(); - log!(logger, "Dropping index `{}` on table `{}`", index_name, table_def.name); + log!(logger, "Dropping index `{index_name}` on table `{table_full_name}`"); stdb.drop_index(tx, index_schema.index_id)?; } spacetimedb_schema::auto_migrate::AutoMigrateStep::RemoveConstraint(constraint_name) => { - let table_def = plan.old.stored_in_table_def(constraint_name).unwrap(); - - let table_id = stdb.table_id_from_name_mut(tx, &table_def.name)?.unwrap(); + let (prefix, _owning_def, table_def, _constraint_def) = plan + .old + .find_constraint_by_full_name(&*constraint_name) + .ok_or_else(|| anyhow::anyhow!("RemoveConstraint: constraint `{constraint_name}` not found in old module def"))?; + let table_full_name = format!("{}{}", prefix, &*table_def.accessor_name); + let table_id = stdb.table_id_from_name_mut(tx, &table_full_name)?.unwrap(); let table_schema = stdb.schema_for_table_mut(tx, table_id)?; + let constraint_schema = table_schema .constraints .iter() - .find(|constraint| constraint.constraint_name[..] == constraint_name[..]) + .find(|constraint| constraint.constraint_name[..] == *constraint_name) .unwrap(); - log!( - logger, - "Dropping constraint `{}` on table `{}`", - constraint_name, - table_def.name - ); + log!(logger, "Dropping constraint `{constraint_name}` on table `{table_full_name}`"); stdb.drop_constraint(tx, constraint_schema.constraint_id)?; } spacetimedb_schema::auto_migrate::AutoMigrateStep::AddSequence(sequence_name) => { - let table_def = plan.new.stored_in_table_def(sequence_name).unwrap(); - let sequence_def = table_def.sequences.get(sequence_name).unwrap(); - - let table_id = stdb.table_id_from_name_mut(tx, &table_def.name)?.unwrap(); + let (prefix, owning_def, table_def, sequence_def) = plan + .new + .find_sequence_by_full_name(&*sequence_name) + .ok_or_else(|| anyhow::anyhow!("AddSequence: sequence `{sequence_name}` not found in new module def"))?; + let table_full_name = format!("{}{}", prefix, &*table_def.accessor_name); + let table_id = stdb.table_id_from_name_mut(tx, &table_full_name)?.unwrap(); let table_schema = stdb.schema_for_table_mut(tx, table_id)?; - log!( - logger, - "Adding sequence `{}` to table `{}`", - sequence_name, - table_def.name - ); + log!(logger, "Adding sequence `{sequence_name}` to table `{table_full_name}`"); let sequence_schema = - SequenceSchema::from_module_def(plan.new, sequence_def, table_schema.table_id, 0.into()); + SequenceSchema::from_module_def(owning_def, sequence_def, table_schema.table_id, 0.into()); stdb.create_sequence(tx, sequence_schema)?; } spacetimedb_schema::auto_migrate::AutoMigrateStep::RemoveSequence(sequence_name) => { - let table_def = plan.old.stored_in_table_def(sequence_name).unwrap(); - - let table_id = stdb.table_id_from_name_mut(tx, &table_def.name)?.unwrap(); + let (prefix, _owning_def, table_def, _sequence_def) = plan + .old + .find_sequence_by_full_name(&*sequence_name) + .ok_or_else(|| anyhow::anyhow!("RemoveSequence: sequence `{sequence_name}` not found in old module def"))?; + let table_full_name = format!("{}{}", prefix, &*table_def.accessor_name); + let table_id = stdb.table_id_from_name_mut(tx, &table_full_name)?.unwrap(); let table_schema = stdb.schema_for_table_mut(tx, table_id)?; let sequence_schema = table_schema .sequences .iter() - .find(|sequence| sequence.sequence_name[..] == sequence_name[..]) + .find(|sequence| sequence.sequence_name[..] == *sequence_name) .unwrap(); - log!( - logger, - "Dropping sequence `{}` from table `{}`", - sequence_name, - table_def.name - ); + log!(logger, "Dropping sequence `{sequence_name}` from table `{table_full_name}`"); stdb.drop_sequence(tx, sequence_schema.sequence_id)?; } spacetimedb_schema::auto_migrate::AutoMigrateStep::ChangeColumns(table_name) => { - let table_def = plan.new.stored_in_table_def(&table_name.clone().into()).unwrap(); - let table_id = stdb.table_id_from_name_mut(tx, table_name).unwrap().unwrap(); - let column_schemas = column_schemas_from_defs(plan.new, &table_def.columns, table_id); + let (_prefix, owning_def, table_def) = plan + .new + .find_table_by_full_name(&*table_name) + .ok_or_else(|| anyhow::anyhow!("ChangeColumns: table `{table_name}` not found in new module def"))?; + let table_id = stdb.table_id_from_name_mut(tx, &*table_name).unwrap().unwrap(); + let column_schemas = column_schemas_from_defs(owning_def, &table_def.columns, table_id); - log!(logger, "Changing columns of table `{}`", table_name); + log!(logger, "Changing columns of table `{table_name}`"); stdb.alter_table_row_type(tx, table_id, column_schemas)?; } spacetimedb_schema::auto_migrate::AutoMigrateStep::ChangeAccess(table_name) => { - 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())?; + let (_prefix, _owning_def, table_def) = plan + .new + .find_table_by_full_name(&*table_name) + .ok_or_else(|| anyhow::anyhow!("ChangeAccess: table `{table_name}` not found in new module def"))?; + stdb.alter_table_access(tx, &*table_name, table_def.table_access.into())?; } spacetimedb_schema::auto_migrate::AutoMigrateStep::ChangePrimaryKey(table_name) => { - let table_def = plan.new.stored_in_table_def(&table_name.clone().into()).unwrap(); + let (_prefix, _owning_def, table_def) = plan + .new + .find_table_by_full_name(&*table_name) + .ok_or_else(|| anyhow::anyhow!("ChangePrimaryKey: table `{table_name}` not found in new module def"))?; log!(logger, "Changing primary key for table `{table_name}`"); - stdb.alter_table_primary_key(tx, table_name, table_def.primary_key)?; + stdb.alter_table_primary_key(tx, &*table_name, table_def.primary_key)?; } spacetimedb_schema::auto_migrate::AutoMigrateStep::AddSchedule(_) => { anyhow::bail!("Adding schedules is not yet implemented"); @@ -295,22 +320,26 @@ fn auto_migrate_database( } spacetimedb_schema::auto_migrate::AutoMigrateStep::AddRowLevelSecurity(sql_rls) => { log!(logger, "Adding row-level security `{sql_rls}`"); - let rls = plan.new.lookup_expect(sql_rls); + let rls = plan + .new + .row_level_security() + .find(|r| &*r.sql == &*sql_rls) + .ok_or_else(|| anyhow::anyhow!("AddRowLevelSecurity: RLS `{sql_rls}` not found in new module def"))?; let rls = RowLevelExpr::build_row_level_expr(tx, &auth_ctx, rls)?; stdb.create_row_level_security(tx, rls.def)?; } spacetimedb_schema::auto_migrate::AutoMigrateStep::RemoveRowLevelSecurity(sql_rls) => { - log!(logger, "Removing-row level security `{sql_rls}`"); - stdb.drop_row_level_security(tx, sql_rls.clone())?; + log!(logger, "Removing row-level security `{sql_rls}`"); + stdb.drop_row_level_security(tx, sql_rls)?; } spacetimedb_schema::auto_migrate::AutoMigrateStep::AddColumns(table_name) => { - let table_def = plan + let (_prefix, owning_def, table_def) = plan .new - .stored_in_table_def(&table_name.clone().into()) - .expect("table must exist"); - let table_id = stdb.table_id_from_name_mut(tx, table_name).unwrap().unwrap(); - let column_schemas = column_schemas_from_defs(plan.new, &table_def.columns, table_id); + .find_table_by_full_name(&*table_name) + .ok_or_else(|| anyhow::anyhow!("AddColumns: table `{table_name}` not found in new module def"))?; + let table_id = stdb.table_id_from_name_mut(tx, &*table_name).unwrap().unwrap(); + let column_schemas = column_schemas_from_defs(owning_def, &table_def.columns, table_id); let default_values: Vec = table_def .columns diff --git a/crates/schema/src/auto_migrate.rs b/crates/schema/src/auto_migrate.rs index f2fba813fa8..4127d2d013d 100644 --- a/crates/schema/src/auto_migrate.rs +++ b/crates/schema/src/auto_migrate.rs @@ -1,13 +1,14 @@ use core::{cmp::Ordering, ops::BitOr}; -use crate::{def::*, error::PrettyAlgebraicType, identifier::Identifier}; +use crate::{def::*, error::PrettyAlgebraicType, identifier::{Identifier, NamespacedIdentifier}}; +use spacetimedb_data_structures::map::HashMap; use formatter::format_plan; use spacetimedb_data_structures::{ error_stream::{CollectAllErrors, CombineErrors, ErrorStream}, map::{HashCollectionExt as _, HashSet}, }; use spacetimedb_lib::{ - db::raw_def::v9::{RawRowLevelSecurityDefV9, TableType}, + db::raw_def::v9::TableType, hash_bytes, Identity, }; use spacetimedb_sats::{ @@ -197,15 +198,15 @@ pub struct AutoMigratePlan<'def> { pub new: &'def ModuleDef, /// The checks to perform before the automatic migration. /// There is also an implied check: that the schema in the database is compatible with the old ModuleDef. - pub prechecks: Vec>, + pub prechecks: Vec, /// The migration steps to perform. /// Order matters: `Remove`s of a particular `Def` must be ordered before `Add`s. - pub steps: Vec>, + pub steps: Vec, } impl AutoMigratePlan<'_> { fn any_step(&self, f: impl Fn(&AutoMigrateStep) -> bool) -> bool { - self.steps.iter().any(f) + self.steps.iter().any(|step| f(step)) } fn disconnects_all_users(&self) -> bool { @@ -224,94 +225,83 @@ impl AutoMigratePlan<'_> { /// Checks that must be performed before performing an automatic migration. /// These checks can access table contents and other database state. #[derive(PartialEq, Eq, Debug, PartialOrd, Ord)] -pub enum AutoMigratePrecheck<'def> { +pub enum AutoMigratePrecheck { /// Perform a check that adding a sequence is valid (the relevant column contains no values /// greater than the sequence's start value). - CheckAddSequenceRangeValid(::Key<'def>), + /// Payload is the full namespaced sequence name (e.g., `"lib.library_table_id_seq"`). + CheckAddSequenceRangeValid(NamespacedIdentifier), } /// A step in an automatic migration. +/// +/// All variant payloads are full namespaced names (e.g., `"lib.library_table"` for a mounted +/// table, or `"user"` for a root-level table). This allows mounted and root-level items to be +/// handled uniformly. Row-level security payloads are SQL text (`Box`) rather than +/// identifiers. +/// +/// IMPORTANT: Remove variants MUST be declared before Add variants in this enum. The ordering +/// is used to sort steps of an auto-migration so that removes precede adds for the same name. #[derive(PartialEq, Eq, Debug, PartialOrd, Ord)] -pub enum AutoMigrateStep<'def> { - // It is important FOR CORRECTNESS that `Remove` variants are declared before `Add` variants in this enum! - // - // The ordering is used to sort the steps of an auto-migration. - // If adds go before removes, and the user tries to remove an index and then re-add it with new configuration, - // the following can occur: - // - // 1. `AddIndex("indexname")` - // 2. `RemoveIndex("indexname")` - // - // This results in the existing index being re-added -- which, at time of writing, does nothing -- and then removed, - // resulting in the intended index not being created. - // - // For now, we just ensure that we declare all `Remove` variants before `Add` variants - // and let `#[derive(PartialOrd)]` take care of the rest. - // - // TODO: when this enum is made serializable, a more durable fix will be needed here. - // Probably we will want to have separate arrays of add and remove steps. - // - /// Remove an index. - RemoveIndex(::Key<'def>), - /// Remove a constraint. - RemoveConstraint(::Key<'def>), - /// Remove a sequence. - RemoveSequence(::Key<'def>), - /// Remove a schedule annotation from a table. - RemoveSchedule(::Key<'def>), - /// Remove a view and corresponding view table - RemoveView(::Key<'def>), - /// Remove a row-level security query. - RemoveRowLevelSecurity(::Key<'def>), +pub enum AutoMigrateStep { + /// Remove an index. Payload is the full namespaced index name. + RemoveIndex(NamespacedIdentifier), + /// Remove a constraint. Payload is the full namespaced constraint name. + RemoveConstraint(NamespacedIdentifier), + /// Remove a sequence. Payload is the full namespaced sequence name. + RemoveSequence(NamespacedIdentifier), + /// Remove a schedule annotation from a table. Payload is the full namespaced TABLE name. + RemoveSchedule(NamespacedIdentifier), + /// Remove a view and corresponding view table. Payload is the full namespaced view name. + RemoveView(NamespacedIdentifier), + /// Remove a row-level security query. Payload is the SQL text. + RemoveRowLevelSecurity(Box), /// Remove an empty table and all its sub-objects (indexes, constraints, sequences). /// Validated at execution time: fails if the table contains data. - RemoveTable(::Key<'def>), + /// Payload is the full namespaced table name (e.g., `"lib.library_table"` or `"user"`). + RemoveTable(NamespacedIdentifier), /// Change the column types of a table, in a layout compatible way. - /// - /// This should be done before any new indices are added. - ChangeColumns(::Key<'def>), + /// Payload is the full namespaced table name. + ChangeColumns(NamespacedIdentifier), /// Add columns to a table, in a layout-INCOMPATIBLE way. /// /// This is a destructive operation that requires first running a `DisconnectAllUsers`. - /// - /// The added columns are guaranteed to be contiguous - /// and at the end of the table. + /// The added columns are guaranteed to be contiguous and at the end of the table. /// They are also guaranteed to have default values set. - /// - /// When this step is present, - /// no `ChangeColumns` steps will be, for the same table. - AddColumns(::Key<'def>), + /// When this step is present, no `ChangeColumns` steps will be, for the same table. + /// Payload is the full namespaced table name. + AddColumns(NamespacedIdentifier), /// Add a table, including all indexes, constraints, and sequences. /// There will NOT be separate steps in the plan for adding indexes, constraints, and sequences. - AddTable(::Key<'def>), - /// Add an index. - AddIndex(::Key<'def>), - /// Add a sequence. - AddSequence(::Key<'def>), - /// Add a schedule annotation to a table. - AddSchedule(::Key<'def>), - /// Add a view and corresponding view table - AddView(::Key<'def>), - /// Add a row-level security query. - AddRowLevelSecurity(::Key<'def>), - - /// Change the access of a table. - ChangeAccess(::Key<'def>), + /// Payload is the full namespaced table name. + AddTable(NamespacedIdentifier), + /// Add an index. Payload is the full namespaced index name. + AddIndex(NamespacedIdentifier), + /// Add a sequence. Payload is the full namespaced sequence name. + AddSequence(NamespacedIdentifier), + /// Add a schedule annotation to a table. Payload is the full namespaced TABLE name. + AddSchedule(NamespacedIdentifier), + /// Add a view and corresponding view table. Payload is the full namespaced view name. + AddView(NamespacedIdentifier), + /// Add a row-level security query. Payload is the SQL text. + AddRowLevelSecurity(Box), + + /// Change the access of a table or view. Payload is the full namespaced table/view name. + ChangeAccess(NamespacedIdentifier), /// Change the primary key of a table. /// - /// This updates the `table_primary_key` field in `st_table` - /// to match the new module definition. - /// Without this step, a stale primary key in the stored schema - /// causes `check_compatible` to fail on the next publish. - /// See: - ChangePrimaryKey(::Key<'def>), + /// This updates the `table_primary_key` field in `st_table` to match the new module definition. + /// Without this step, a stale primary key in the stored schema causes `check_compatible` to + /// fail on the next publish. See: + /// Payload is the full namespaced table name. + ChangePrimaryKey(NamespacedIdentifier), - /// Recompute a view, update its backing table, and push updates to clients - UpdateView(::Key<'def>), + /// Recompute a view, update its backing table, and push updates to clients. + /// Payload is the full namespaced view name. + UpdateView(NamespacedIdentifier), /// Disconnect all users connected to the module. DisconnectAllUsers, @@ -470,26 +460,26 @@ pub fn ponder_auto_migrate<'def>(old: &'def ModuleDef, new: &'def ModuleDef) -> let views_ok = auto_migrate_views(&mut plan); let tables_ok = auto_migrate_tables(&mut plan); - // Filter out sub-objects of added/removed tables — they're handled by `AddTable`/`RemoveTable`. - let (new_tables, removed_tables): (HashSet<&Identifier>, HashSet<&Identifier>) = - diff(plan.old, plan.new, ModuleDef::tables).fold( - (HashSet::new(), HashSet::new()), - |(mut added, mut removed), diff| { - match diff { - Diff::Add { new } => { - added.insert(&new.name); - } - Diff::Remove { old } => { - removed.insert(&old.name); - } - Diff::MaybeChange { .. } => {} - } - (added, removed) - }, - ); - let indexes_ok = auto_migrate_indexes(&mut plan, &new_tables, &removed_tables); - let sequences_ok = auto_migrate_sequences(&mut plan, &new_tables, &removed_tables); - let constraints_ok = auto_migrate_constraints(&mut plan, &new_tables, &removed_tables); + // Compute full-name sets for added/removed tables (across all mounts). + // Sub-objects of added/removed tables are handled by AddTable/RemoveTable, not individually. + let old_module = plan.old; + let new_module = plan.new; + let old_table_names: HashSet = old_module + .all_tables_with_prefix() + .into_iter() + .map(|(prefix, _, t)| format!("{}{}", prefix, &*t.accessor_name)) + .collect(); + let new_table_names: HashSet = new_module + .all_tables_with_prefix() + .into_iter() + .map(|(prefix, _, t)| format!("{}{}", prefix, &*t.accessor_name)) + .collect(); + let added_tables: HashSet = new_table_names.difference(&old_table_names).cloned().collect(); + let removed_tables: HashSet = old_table_names.difference(&new_table_names).cloned().collect(); + + let indexes_ok = auto_migrate_indexes(&mut plan, &added_tables, &removed_tables); + let sequences_ok = auto_migrate_sequences(&mut plan, &added_tables, &removed_tables); + let constraints_ok = auto_migrate_constraints(&mut plan, &added_tables, &removed_tables); // IMPORTANT: RLS auto-migrate steps must come last, // since they assume that any schema changes, like adding or dropping tables, // have already been reflected in the database state. @@ -504,166 +494,230 @@ pub fn ponder_auto_migrate<'def>(old: &'def ModuleDef, new: &'def ModuleDef) -> Ok(plan) } -/// A diff between two items. -/// `Add` means the item is present in the new `ModuleDef` but not the old. -/// `Remove` means the item is present in the old `ModuleDef` but not the new. -/// `MaybeChange` indicates the item is present in both. -#[derive(Debug)] -enum Diff<'def, T> { - Add { new: &'def T }, - Remove { old: &'def T }, - MaybeChange { old: &'def T, new: &'def T }, -} - -/// Diff a collection of items, looking them up in both the old and new `ModuleDef` by their `ModuleDefLookup::Key`. -/// Keys are required to be stable across migrations, which makes this possible. -fn diff<'def, T: ModuleDefLookup, I: Iterator>( - old: &'def ModuleDef, - new: &'def ModuleDef, - iter: impl Fn(&'def ModuleDef) -> I, -) -> impl Iterator> { - iter(old) - .map(move |old_item| match T::lookup(new, old_item.key()) { - Some(new_item) => Diff::MaybeChange { - old: old_item, - new: new_item, - }, - None => Diff::Remove { old: old_item }, - }) - .chain(iter(new).filter_map(move |new_item| { - if T::lookup(old, new_item.key()).is_none() { - Some(Diff::Add { new: new_item }) - } else { - None - } - })) -} - fn auto_migrate_views(plan: &mut AutoMigratePlan<'_>) -> Result<()> { - diff(plan.old, plan.new, ModuleDef::views) - .map(|table_diff| -> Result<()> { - match table_diff { - Diff::Add { new } => { - plan.steps.push(AutoMigrateStep::AddView(new.key())); - Ok(()) - } - // From the user's perspective, views do not have persistent state. - // Hence removal does not require a manual migration - just disconnecting clients. - Diff::Remove { old } => { - plan.steps.push(AutoMigrateStep::RemoveView(old.key())); - plan.ensure_disconnect_all_users(); - Ok(()) - } - Diff::MaybeChange { old, new } => auto_migrate_view(plan, old, new), + let old_module = plan.old; + let new_module = plan.new; + + // Build full-name maps for views across all mounts. + let old_views: HashMap = old_module + .all_views_with_prefix() + .into_iter() + .map(|(prefix, owning, view)| (format!("{}{}", prefix, &*view.name), (owning, view))) + .collect(); + let new_views: HashMap = new_module + .all_views_with_prefix() + .into_iter() + .map(|(prefix, owning, view)| (format!("{}{}", prefix, &*view.name), (owning, view))) + .collect(); + + let mut maybe_change: Vec<(String, (&ModuleDef, &ViewDef), (&ModuleDef, &ViewDef))> = vec![]; + for (full_name, old_entry) in &old_views { + match new_views.get(full_name.as_str()) { + Some(new_entry) => maybe_change.push((full_name.clone(), *old_entry, *new_entry)), + None => { + plan.steps + .push(AutoMigrateStep::RemoveView(NamespacedIdentifier::new_assume_valid(full_name.clone()))); + plan.ensure_disconnect_all_users(); } + } + } + for (full_name, _) in &new_views { + if !old_views.contains_key(full_name.as_str()) { + plan.steps + .push(AutoMigrateStep::AddView(NamespacedIdentifier::new_assume_valid(full_name.clone()))); + } + } + + let results: Vec> = maybe_change + .into_iter() + .map(|(full_name, (old_owning, old_view), (new_owning, new_view))| { + auto_migrate_view( + plan, + NamespacedIdentifier::new_assume_valid(full_name), + old_owning, + old_view, + new_owning, + new_view, + ) }) - .collect_all_errors() + .collect(); + results.into_iter().collect_all_errors::>().map(|_| ()) } -fn auto_migrate_view<'def>(plan: &mut AutoMigratePlan<'def>, old: &'def ViewDef, new: &'def ViewDef) -> Result<()> { - let key = old.key(); - - if old.is_public != new.is_public { - plan.steps.push(AutoMigrateStep::ChangeAccess(key)); - } - +fn auto_migrate_view( + plan: &mut AutoMigratePlan, + full_name: NamespacedIdentifier, + old_owning: &ModuleDef, + old: &ViewDef, + new_owning: &ModuleDef, + new: &ViewDef, +) -> Result<()> { // We can always auto-migrate a view because we can always re-compute it. // However certain things require us to disconnect clients: // 1. If we add or remove a column or parameter // 2. If we change the order of the columns or parameters // 3. If we change the types of the columns or parameters // 4. If we change the context parameter - let Any(incompatible_return_type) = diff(plan.old, plan.new, |def| { - def.lookup_expect::(key).return_columns.iter() - }) - .map(|col_diff| { - match col_diff { - // We must disconnect clients if we add or remove a parameter or column - Diff::Add { .. } | Diff::Remove { .. } => Any(true), - Diff::MaybeChange { old, new } => { - if old.col_id != new.col_id { + let old_return_cols: HashMap<&Identifier, &ViewColumnDef> = old.return_columns.iter().map(|c| (&c.name, c)).collect(); + let new_return_cols: HashMap<&Identifier, &ViewColumnDef> = new.return_columns.iter().map(|c| (&c.name, c)).collect(); + + let Any(incompatible_return_type) = old + .return_columns + .iter() + .map(|old_col| match new_return_cols.get(&old_col.name) { + None => Any(true), + Some(new_col) => { + if old_col.col_id != new_col.col_id { return Any(true); - }; - + } ensure_old_ty_upgradable_to_new( false, - &|| old.view_name.clone(), - &|| old.name.clone(), - &WithTypespace::new(plan.old.typespace(), &old.ty) + &|| old_col.view_name.clone(), + &|| old_col.name.clone(), + &WithTypespace::new(old_owning.typespace(), &old_col.ty) .resolve_refs() .expect("valid ViewDefs must have valid type refs"), - &WithTypespace::new(plan.new.typespace(), &new.ty) + &WithTypespace::new(new_owning.typespace(), &new_col.ty) .resolve_refs() .expect("valid ViewDefs must have valid type refs"), ) .unwrap_or(Any(true)) } - } - }) - .collect(); - - let Any(incompatible_param_types) = diff(plan.old, plan.new, |def| { - def.lookup_expect::(key).param_columns.iter() - }) - .map(|col_diff| { - match col_diff { - // We must disconnect clients if we add or remove a parameter or column - Diff::Add { .. } | Diff::Remove { .. } => Any(true), - Diff::MaybeChange { old, new } => { - if old.col_id != new.col_id { + }) + .chain(new.return_columns.iter().map(|new_col| { + if old_return_cols.contains_key(&new_col.name) { + Any(false) + } else { + Any(true) // added column → incompatible + } + })) + .collect(); + + let old_param_cols: HashMap<&Identifier, &ViewParamDef> = old.param_columns.iter().map(|c| (&c.name, c)).collect(); + let new_param_cols: HashMap<&Identifier, &ViewParamDef> = new.param_columns.iter().map(|c| (&c.name, c)).collect(); + + let Any(incompatible_param_types) = old + .param_columns + .iter() + .map(|old_col| match new_param_cols.get(&old_col.name) { + None => Any(true), + Some(new_col) => { + if old_col.col_id != new_col.col_id { return Any(true); - }; - + } ensure_old_ty_upgradable_to_new( false, - &|| old.view_name.clone(), - &|| old.name.clone(), - &WithTypespace::new(plan.old.typespace(), &old.ty) + &|| old_col.view_name.clone(), + &|| old_col.name.clone(), + &WithTypespace::new(old_owning.typespace(), &old_col.ty) .resolve_refs() .expect("valid ViewDefs must have valid type refs"), - &WithTypespace::new(plan.new.typespace(), &new.ty) + &WithTypespace::new(new_owning.typespace(), &new_col.ty) .resolve_refs() .expect("valid ViewDefs must have valid type refs"), ) .unwrap_or(Any(true)) } - } - }) - .collect(); - - if old.is_anonymous != new.is_anonymous || incompatible_return_type || incompatible_param_types { - plan.steps.push(AutoMigrateStep::AddView(new.key())); - plan.steps.push(AutoMigrateStep::RemoveView(old.key())); - + }) + .chain(new.param_columns.iter().map(|new_col| { + if old_param_cols.contains_key(&new_col.name) { + Any(false) + } else { + Any(true) + } + })) + .collect(); + + if old.is_anonymous != new.is_anonymous + || old.is_public != new.is_public + || incompatible_return_type + || incompatible_param_types + { + plan.steps.push(AutoMigrateStep::AddView(full_name.clone())); + plan.steps.push(AutoMigrateStep::RemoveView(full_name)); plan.ensure_disconnect_all_users(); } else { - plan.steps.push(AutoMigrateStep::UpdateView(new.key())); + plan.steps.push(AutoMigrateStep::UpdateView(full_name)); } Ok(()) } fn auto_migrate_tables(plan: &mut AutoMigratePlan<'_>) -> Result<()> { - diff(plan.old, plan.new, ModuleDef::tables) - .map(|table_diff| -> Result<()> { - match table_diff { - Diff::Add { new } => { - plan.steps.push(AutoMigrateStep::AddTable(new.key())); - Ok(()) - } - Diff::Remove { old } => { - plan.steps.push(AutoMigrateStep::RemoveTable(old.key())); - plan.ensure_disconnect_all_users(); - Ok(()) - } - Diff::MaybeChange { old, new } => auto_migrate_table(plan, old, new), - } + let old_module = plan.old; + let new_module = plan.new; + + // Map canonical name (table_def.name, after case conversion) → (accessor full name, owning, table). + // Matching by canonical name means tables like `Events` (accessor) / `events` (canonical) in + // the old module and `events` (accessor + canonical) in the new module are treated as the same + // logical table, preventing spurious Remove+Add steps when only the accessor casing changed. + // Step payloads use accessor names (prefix + accessor_name) since that's what the DB stores. + let old_tables: HashMap = old_module + .all_tables_with_prefix() + .into_iter() + .map(|(prefix, owning, table)| { + let canonical = format!("{}{}", prefix, &*table.name); + let accessor = format!("{}{}", prefix, &*table.accessor_name); + (canonical, (accessor, owning, table)) + }) + .collect(); + let new_tables: HashMap = new_module + .all_tables_with_prefix() + .into_iter() + .map(|(prefix, owning, table)| { + let canonical = format!("{}{}", prefix, &*table.name); + let accessor = format!("{}{}", prefix, &*table.accessor_name); + (canonical, (accessor, owning, table)) + }) + .collect(); + + for (canonical, (accessor, _, _)) in &old_tables { + if !new_tables.contains_key(canonical.as_str()) { + plan.steps + .push(AutoMigrateStep::RemoveTable(NamespacedIdentifier::new_assume_valid(accessor.clone()))); + plan.ensure_disconnect_all_users(); + } + } + for (canonical, (accessor, _, _)) in &new_tables { + if !old_tables.contains_key(canonical.as_str()) { + plan.steps + .push(AutoMigrateStep::AddTable(NamespacedIdentifier::new_assume_valid(accessor.clone()))); + } + } + + let maybe_change: Vec<(String, (&ModuleDef, &TableDef), (&ModuleDef, &TableDef))> = old_tables + .iter() + .filter_map(|(canonical, (_, old_owning, old_table))| { + new_tables + .get(canonical.as_str()) + .map(|(new_accessor, new_owning, new_table)| (new_accessor.clone(), (*old_owning, *old_table), (*new_owning, *new_table))) + }) + .collect(); + + let results: Vec> = maybe_change + .into_iter() + .map(|(full_name, (old_owning, old_table), (new_owning, new_table))| { + auto_migrate_table( + plan, + NamespacedIdentifier::new_assume_valid(full_name), + old_owning, + old_table, + new_owning, + new_table, + ) }) - .collect_all_errors() + .collect(); + results.into_iter().collect_all_errors::>().map(|_| ()) } -fn auto_migrate_table<'def>(plan: &mut AutoMigratePlan<'def>, old: &'def TableDef, new: &'def TableDef) -> Result<()> { - let key = old.key(); +fn auto_migrate_table( + plan: &mut AutoMigratePlan, + full_name: NamespacedIdentifier, + old_owning: &ModuleDef, + old: &TableDef, + new_owning: &ModuleDef, + new: &TableDef, +) -> Result<()> { let type_ok: Result<()> = if old.table_type == new.table_type { Ok(()) } else { @@ -677,88 +731,78 @@ fn auto_migrate_table<'def>(plan: &mut AutoMigratePlan<'def>, old: &'def TableDe let event_ok: Result<()> = if old.is_event == new.is_event { Ok(()) } else { - Err(AutoMigrateError::ChangeTableEventFlag { - table: old.name.clone(), - } - .into()) + Err(AutoMigrateError::ChangeTableEventFlag { table: old.name.clone() }.into()) }; if old.table_access != new.table_access { - plan.steps.push(AutoMigrateStep::ChangeAccess(key)); + plan.steps.push(AutoMigrateStep::ChangeAccess(full_name.clone())); } if old.primary_key != new.primary_key { - plan.steps.push(AutoMigrateStep::ChangePrimaryKey(key)); + plan.steps.push(AutoMigrateStep::ChangePrimaryKey(full_name.clone())); } if old.schedule != new.schedule { - // Note: this handles the case where there's an altered ScheduleDef for some reason. - if let Some(old_schedule) = old.schedule.as_ref() { - plan.steps.push(AutoMigrateStep::RemoveSchedule(old_schedule.key())); + // Schedule steps are keyed by full TABLE name (schedules are 1:1 with tables). + if old.schedule.is_some() { + plan.steps.push(AutoMigrateStep::RemoveSchedule(full_name.clone())); } - if let Some(new_schedule) = new.schedule.as_ref() { - plan.steps.push(AutoMigrateStep::AddSchedule(new_schedule.key())); + if new.schedule.is_some() { + plan.steps.push(AutoMigrateStep::AddSchedule(full_name.clone())); } } - let columns_ok = diff(plan.old, plan.new, |def| { - def.lookup_expect::(key).columns.iter() - }) - .map(|col_diff| -> Result<_> { - match col_diff { - Diff::Add { new } => { - if new.default_value.is_some() { - // `row_type_changed`, `columns_added` - Ok(ProductMonoid(Any(false), Any(true))) - } else { - Err(AutoMigrateError::AddColumn { - table: new.table_name.clone(), - column: new.name.clone(), - } - .into()) + // Diff columns directly using the table defs (avoids root-only ModuleDefLookup). + let new_col_by_name: HashMap<&Identifier, &ColumnDef> = new.columns.iter().map(|c| (&c.name, c)).collect(); + let old_col_by_name: HashMap<&Identifier, &ColumnDef> = old.columns.iter().map(|c| (&c.name, c)).collect(); + + let columns_ok = old + .columns + .iter() + .map(|old_col| -> Result> { + match new_col_by_name.get(&old_col.name) { + None => Err(AutoMigrateError::RemoveColumn { + table: old_col.table_name.clone(), + column: old_col.name.clone(), + } + .into()), + Some(new_col) => { + let old_ty = WithTypespace::new(old_owning.typespace(), &old_col.ty) + .resolve_refs() + .expect("valid TableDef must have valid type refs"); + let new_ty = WithTypespace::new(new_owning.typespace(), &new_col.ty) + .resolve_refs() + .expect("valid TableDef must have valid type refs"); + let types_ok = ensure_old_ty_upgradable_to_new( + false, + &|| old_col.table_name.clone(), + &|| old_col.name.clone(), + &old_ty, + &new_ty, + ); + // Reject reordering of existing columns. + let positions_ok = if old_col.col_id == new_col.col_id { + Ok(()) + } else { + Err(AutoMigrateError::ReorderTable { table: old_col.table_name.clone() }.into()) + }; + (types_ok, positions_ok) + .combine_errors() + .map(|(x, _)| ProductMonoid(x, Any(false))) } } - Diff::Remove { old } => Err(AutoMigrateError::RemoveColumn { - table: old.table_name.clone(), - column: old.name.clone(), - } - .into()), - Diff::MaybeChange { old, new } => { - // Check column type upgradability. - let old_ty = WithTypespace::new(plan.old.typespace(), &old.ty) - .resolve_refs() - .expect("valid TableDef must have valid type refs"); - let new_ty = WithTypespace::new(plan.new.typespace(), &new.ty) - .resolve_refs() - .expect("valid TableDef must have valid type refs"); - let types_ok = ensure_old_ty_upgradable_to_new( - false, - &|| old.table_name.clone(), - &|| old.name.clone(), - &old_ty, - &new_ty, - ); - - // Note that the diff algorithm relies on `ModuleDefLookup` for `ColumnDef`, - // which looks up columns by NAME, NOT position: precisely to allow this step to work! - - // Note: We reject changes to positions. This means that, if a column was present in the old version of the table, - // it must be in the same place in the new version of the table. - // This guarantees that any added columns live at the end of the table. - let positions_ok = if old.col_id == new.col_id { - Ok(()) - } else { - Err(AutoMigrateError::ReorderTable { - table: old.table_name.clone(), - } - .into()) - }; - - (types_ok, positions_ok) - .combine_errors() - // row_type_changed, column_added - .map(|(x, _)| ProductMonoid(x, Any(false))) + }) + .chain(new.columns.iter().map(|new_col| -> Result> { + if old_col_by_name.contains_key(&new_col.name) { + Ok(ProductMonoid(Any(false), Any(false))) + } else if new_col.default_value.is_some() { + Ok(ProductMonoid(Any(false), Any(true))) + } else { + Err(AutoMigrateError::AddColumn { + table: new_col.table_name.clone(), + column: new_col.name.clone(), + } + .into()) } - } - }) - .collect_all_errors::>(); + })) + .collect_all_errors::>(); let ((), (), ProductMonoid(Any(row_type_changed), Any(columns_added))) = (type_ok, event_ok, columns_ok).combine_errors()?; @@ -767,9 +811,9 @@ fn auto_migrate_table<'def>(plan: &mut AutoMigratePlan<'def>, old: &'def TableDe // That makes any `ChangeColumns` moot, so we can skip it. if columns_added { plan.ensure_disconnect_all_users(); - plan.steps.push(AutoMigrateStep::AddColumns(key)); + plan.steps.push(AutoMigrateStep::AddColumns(full_name)); } else if row_type_changed { - plan.steps.push(AutoMigrateStep::ChangeColumns(key)); + plan.steps.push(AutoMigrateStep::ChangeColumns(full_name)); } Ok(()) @@ -951,141 +995,249 @@ fn ensure_old_ty_upgradable_to_new( fn auto_migrate_indexes( plan: &mut AutoMigratePlan<'_>, - new_tables: &HashSet<&Identifier>, - removed_tables: &HashSet<&Identifier>, + new_tables: &HashSet, + removed_tables: &HashSet, ) -> Result<()> { - diff(plan.old, plan.new, ModuleDef::indexes) - .map(|index_diff| -> Result<()> { - match index_diff { - Diff::Add { new } => { - if !new_tables.contains(&plan.new.stored_in_table_def(&new.name).unwrap().name) { - plan.steps.push(AutoMigrateStep::AddIndex(new.key())); - } - Ok(()) - } - Diff::Remove { old } => { - if !removed_tables.contains(&plan.old.stored_in_table_def(&old.name).unwrap().name) { - plan.steps.push(AutoMigrateStep::RemoveIndex(old.key())); - } - Ok(()) + let old_module = plan.old; + let new_module = plan.new; + + // key = full index name (e.g. "lib.library_table_id_idx_btree") + // value = (full_table_name, &IndexDef) + let old_indexes: HashMap = { + let mut map = HashMap::new(); + for (prefix, _, table) in old_module.all_tables_with_prefix() { + let table_full = format!("{}{}", prefix, &*table.accessor_name); + for idx in table.indexes.values() { + map.insert(format!("{}{}", prefix, &*idx.name), (table_full.clone(), idx)); + } + } + map + }; + let new_indexes: HashMap = { + let mut map = HashMap::new(); + for (prefix, _, table) in new_module.all_tables_with_prefix() { + let table_full = format!("{}{}", prefix, &*table.accessor_name); + for idx in table.indexes.values() { + map.insert(format!("{}{}", prefix, &*idx.name), (table_full.clone(), idx)); + } + } + map + }; + + // Removed indexes: in old but not in new, and not part of a removed table. + for (full_idx_name, (table_full_name, _)) in &old_indexes { + if !new_indexes.contains_key(full_idx_name.as_str()) && !removed_tables.contains(table_full_name) { + plan.steps.push(AutoMigrateStep::RemoveIndex( + NamespacedIdentifier::new_assume_valid(full_idx_name.clone()), + )); + } + } + + // Added indexes: in new but not in old, and not part of a newly added table. + for (full_idx_name, (table_full_name, _)) in &new_indexes { + if !old_indexes.contains_key(full_idx_name.as_str()) && !new_tables.contains(table_full_name) { + plan.steps.push(AutoMigrateStep::AddIndex( + NamespacedIdentifier::new_assume_valid(full_idx_name.clone()), + )); + } + } + + // Changed indexes: same name in both. + let change_results: Vec> = old_indexes + .iter() + .filter_map(|(full_idx_name, (_, old_idx))| { + new_indexes.get(full_idx_name.as_str()).map(|(_, new_idx)| (full_idx_name, old_idx, new_idx)) + }) + .map(|(full_idx_name, old_idx, new_idx)| { + if old_idx.accessor_name != new_idx.accessor_name { + Err(AutoMigrateError::ChangeIndexAccessor { + index: old_idx.name.clone(), + old_accessor: old_idx.accessor_name.clone(), + new_accessor: new_idx.accessor_name.clone(), } - Diff::MaybeChange { old, new } => { - if old.accessor_name != new.accessor_name { - Err(AutoMigrateError::ChangeIndexAccessor { - index: old.name.clone(), - old_accessor: old.accessor_name.clone(), - new_accessor: new.accessor_name.clone(), - } - .into()) - } else { - if old.algorithm != new.algorithm { - plan.steps.push(AutoMigrateStep::RemoveIndex(old.key())); - plan.steps.push(AutoMigrateStep::AddIndex(old.key())); - } - Ok(()) - } + .into()) + } else { + if old_idx.algorithm != new_idx.algorithm { + plan.steps.push(AutoMigrateStep::RemoveIndex( + NamespacedIdentifier::new_assume_valid(full_idx_name.clone()), + )); + plan.steps.push(AutoMigrateStep::AddIndex( + NamespacedIdentifier::new_assume_valid(full_idx_name.clone()), + )); } + Ok(()) } }) - .collect_all_errors() + .collect(); + change_results.into_iter().collect_all_errors::>().map(|_| ()) } fn auto_migrate_sequences( plan: &mut AutoMigratePlan, - new_tables: &HashSet<&Identifier>, - removed_tables: &HashSet<&Identifier>, + new_tables: &HashSet, + removed_tables: &HashSet, ) -> Result<()> { - diff(plan.old, plan.new, ModuleDef::sequences) - .map(|sequence_diff| -> Result<()> { - match sequence_diff { - Diff::Add { new } => { - if !new_tables.contains(&plan.new.stored_in_table_def(&new.name).unwrap().name) { - plan.prechecks - .push(AutoMigratePrecheck::CheckAddSequenceRangeValid(new.key())); - plan.steps.push(AutoMigrateStep::AddSequence(new.key())); - } - Ok(()) - } - Diff::Remove { old } => { - if !removed_tables.contains(&plan.old.stored_in_table_def(&old.name).unwrap().name) { - plan.steps.push(AutoMigrateStep::RemoveSequence(old.key())); - } - Ok(()) - } - Diff::MaybeChange { old, new } => { - // we do not need to check column ids, since in an automigrate, column ids are not changed. - if old != new { - plan.prechecks - .push(AutoMigratePrecheck::CheckAddSequenceRangeValid(new.key())); - plan.steps.push(AutoMigrateStep::RemoveSequence(old.key())); - plan.steps.push(AutoMigrateStep::AddSequence(new.key())); - } - Ok(()) - } + let old_module = plan.old; + let new_module = plan.new; + + // key = full sequence name (e.g. "lib.Bananas_id_seq") + // value = (full_table_name, &SequenceDef) + let old_seqs: HashMap = { + let mut map = HashMap::new(); + for (prefix, _, table) in old_module.all_tables_with_prefix() { + let table_full = format!("{}{}", prefix, &*table.accessor_name); + for seq in table.sequences.values() { + map.insert(format!("{}{}", prefix, &*seq.name), (table_full.clone(), seq)); } - }) - .collect_all_errors() + } + map + }; + let new_seqs: HashMap = { + let mut map = HashMap::new(); + for (prefix, _, table) in new_module.all_tables_with_prefix() { + let table_full = format!("{}{}", prefix, &*table.accessor_name); + for seq in table.sequences.values() { + map.insert(format!("{}{}", prefix, &*seq.name), (table_full.clone(), seq)); + } + } + map + }; + + // Removed sequences: in old but not in new, and not part of a removed table. + for (full_seq_name, (table_full_name, _)) in &old_seqs { + if !new_seqs.contains_key(full_seq_name.as_str()) && !removed_tables.contains(table_full_name) { + plan.steps.push(AutoMigrateStep::RemoveSequence( + NamespacedIdentifier::new_assume_valid(full_seq_name.clone()), + )); + } + } + + // Added or changed sequences. + for (full_seq_name, (table_full_name, new_seq)) in &new_seqs { + if let Some((_, old_seq)) = old_seqs.get(full_seq_name.as_str()) { + // we do not need to check column ids, since in an automigrate, column ids are not changed. + if *old_seq != *new_seq { + plan.prechecks.push(AutoMigratePrecheck::CheckAddSequenceRangeValid( + NamespacedIdentifier::new_assume_valid(full_seq_name.clone()), + )); + plan.steps.push(AutoMigrateStep::RemoveSequence( + NamespacedIdentifier::new_assume_valid(full_seq_name.clone()), + )); + plan.steps.push(AutoMigrateStep::AddSequence( + NamespacedIdentifier::new_assume_valid(full_seq_name.clone()), + )); + } + } else if !new_tables.contains(table_full_name) { + plan.prechecks.push(AutoMigratePrecheck::CheckAddSequenceRangeValid( + NamespacedIdentifier::new_assume_valid(full_seq_name.clone()), + )); + plan.steps.push(AutoMigrateStep::AddSequence( + NamespacedIdentifier::new_assume_valid(full_seq_name.clone()), + )); + } + } + + Ok(()) } fn auto_migrate_constraints( plan: &mut AutoMigratePlan, - new_tables: &HashSet<&Identifier>, - removed_tables: &HashSet<&Identifier>, + new_tables: &HashSet, + removed_tables: &HashSet, ) -> Result<()> { - diff(plan.old, plan.new, ModuleDef::constraints) - .map(|constraint_diff| -> Result<()> { - match constraint_diff { - Diff::Add { new } => { - if new_tables.contains(&plan.new.stored_in_table_def(&new.name).unwrap().name) { - // it's okay to add a constraint in a new table. - Ok(()) - } else { - // it's not okay to add a new constraint to an existing table. - Err(AutoMigrateError::AddUniqueConstraint { - constraint: new.name.clone(), - } - .into()) - } - } - Diff::Remove { old } => { - if !removed_tables.contains(&plan.old.stored_in_table_def(&old.name).unwrap().name) { - plan.steps.push(AutoMigrateStep::RemoveConstraint(old.key())); - } - Ok(()) + let old_module = plan.old; + let new_module = plan.new; + + // key = full constraint name (e.g. "lib.Apples_id_key") + // value = (full_table_name, &ConstraintDef) + let old_constraints: HashMap = { + let mut map = HashMap::new(); + for (prefix, _, table) in old_module.all_tables_with_prefix() { + let table_full = format!("{}{}", prefix, &*table.accessor_name); + for constraint in table.constraints.values() { + map.insert( + format!("{}{}", prefix, &*constraint.name), + (table_full.clone(), constraint), + ); + } + } + map + }; + let new_constraints: HashMap = { + let mut map = HashMap::new(); + for (prefix, _, table) in new_module.all_tables_with_prefix() { + let table_full = format!("{}{}", prefix, &*table.accessor_name); + for constraint in table.constraints.values() { + map.insert( + format!("{}{}", prefix, &*constraint.name), + (table_full.clone(), constraint), + ); + } + } + map + }; + + let mut results: Vec> = vec![]; + + // Added constraints. + for (full_constraint_name, (table_full_name, new_constraint)) in &new_constraints { + if !old_constraints.contains_key(full_constraint_name.as_str()) { + if !new_tables.contains(table_full_name) { + // it's not okay to add a new constraint to an existing table. + results.push(Err(AutoMigrateError::AddUniqueConstraint { + constraint: new_constraint.name.clone(), } - Diff::MaybeChange { old, new } => { - if old == new { - Ok(()) - } else { - Err(AutoMigrateError::ChangeUniqueConstraint { - constraint: old.name.clone(), - } - .into()) - } + .into())); + } + // it's okay to add a constraint in a new table — AddTable covers it. + } + } + + // Removed constraints: not part of a removed table. + for (full_constraint_name, (table_full_name, _)) in &old_constraints { + if !new_constraints.contains_key(full_constraint_name.as_str()) && !removed_tables.contains(table_full_name) { + plan.steps.push(AutoMigrateStep::RemoveConstraint( + NamespacedIdentifier::new_assume_valid(full_constraint_name.clone()), + )); + } + } + + // Changed constraints. + for (full_constraint_name, (_, new_constraint)) in &new_constraints { + if let Some((_, old_constraint)) = old_constraints.get(full_constraint_name.as_str()) { + if *old_constraint != *new_constraint { + results.push(Err(AutoMigrateError::ChangeUniqueConstraint { + constraint: old_constraint.name.clone(), } + .into())); } - }) - .collect_all_errors() + } + } + + results.into_iter().collect_all_errors::>().map(|_| ()) } // Because we can refer to many tables and fields on the row level-security query, we need to remove all of them, // then add the new ones, instead of trying to track the graph of dependencies. fn auto_migrate_row_level_security(plan: &mut AutoMigratePlan) -> Result<()> { - // Track if any RLS rules were changed. - let mut old_rls = HashSet::new(); - let mut new_rls = HashSet::new(); + let old_sqls: Vec> = plan.old.row_level_security().map(|rls| rls.sql.clone()).collect(); + let new_sqls: Vec> = plan.new.row_level_security().map(|rls| rls.sql.clone()).collect(); + + let changed = { + let old_set: HashSet<&str> = old_sqls.iter().map(|s| &**s).collect(); + let new_set: HashSet<&str> = new_sqls.iter().map(|s| &**s).collect(); + old_set != new_set + }; - for rls in plan.old.row_level_security() { - old_rls.insert(rls.key()); - plan.steps.push(AutoMigrateStep::RemoveRowLevelSecurity(rls.key())); + for sql in old_sqls { + plan.steps.push(AutoMigrateStep::RemoveRowLevelSecurity(sql)); } - for rls in plan.new.row_level_security() { - new_rls.insert(rls.key()); - plan.steps.push(AutoMigrateStep::AddRowLevelSecurity(rls.key())); + for sql in new_sqls { + plan.steps.push(AutoMigrateStep::AddRowLevelSecurity(sql)); } // We can force flush the cache by force disconnecting all clients if an RLS rule has been added, removed, or updated. - if old_rls != new_rls { + if changed { plan.ensure_disconnect_all_users(); } @@ -1101,7 +1253,7 @@ mod tests { AlgebraicType, AlgebraicValue, ProductType, ScheduleAt, }; use spacetimedb_primitives::ColId; - use v10::{ExplicitNames, RawModuleDefV10Builder}; + use v10::{ExplicitNames, RawModuleDefV10Builder, RawModuleDefV10Section, RawModuleMountV10}; use v9::{RawModuleDefV9Builder, TableAccess}; use validate::tests::expect_identifier; @@ -1332,95 +1484,132 @@ mod tests { let new_def = updated_module_def(); let plan = ponder_auto_migrate(&old_def, &new_def).expect("auto migration should succeed"); - let apples = expect_identifier("Apples"); - let bananas = expect_identifier("Bananas"); - let deliveries = expect_identifier("Deliveries"); - let oranges = expect_identifier("Oranges"); - let my_view = expect_identifier("my_view"); - - let bananas_sequence: RawIdentifier = "Bananas_id_seq".into(); - let apples_unique_constraint: RawIdentifier = "Apples_id_key".into(); - let apples_sequence: RawIdentifier = "Apples_id_seq".into(); - let apples_id_name_index: RawIdentifier = "Apples_id_name_idx_btree".into(); - let apples_id_count_index: RawIdentifier = "Apples_id_count_idx_btree".into(); - let deliveries_schedule = expect_identifier("Deliveries_sched"); - let inspections_schedule = expect_identifier("Inspections_sched"); - assert!(plan.prechecks.is_sorted()); assert_eq!(plan.prechecks.len(), 1); assert_eq!( plan.prechecks[0], - AutoMigratePrecheck::CheckAddSequenceRangeValid(&bananas_sequence) + AutoMigratePrecheck::CheckAddSequenceRangeValid(NamespacedIdentifier::new_assume_valid( + "Bananas_id_seq" + )) ); - let sql_old = RawRowLevelSecurityDefV9 { - sql: "SELECT * FROM Apples".into(), - }; - - let sql_new = RawRowLevelSecurityDefV9 { - sql: "SELECT * FROM Bananas".into(), - }; let steps = &plan.steps[..]; assert!(steps.is_sorted()); assert!( - steps.contains(&AutoMigrateStep::RemoveSequence(&apples_sequence)), + steps.contains(&AutoMigrateStep::RemoveSequence(NamespacedIdentifier::new_assume_valid( + "Apples_id_seq" + ))), "{steps:?}" ); assert!( - steps.contains(&AutoMigrateStep::RemoveConstraint(&apples_unique_constraint)), + steps.contains(&AutoMigrateStep::RemoveConstraint( + NamespacedIdentifier::new_assume_valid("Apples_id_key") + )), "{steps:?}" ); assert!( - steps.contains(&AutoMigrateStep::RemoveIndex(&apples_id_name_index)), + steps.contains(&AutoMigrateStep::RemoveIndex(NamespacedIdentifier::new_assume_valid( + "Apples_id_name_idx_btree" + ))), "{steps:?}" ); assert!( - steps.contains(&AutoMigrateStep::AddIndex(&apples_id_count_index)), + steps.contains(&AutoMigrateStep::AddIndex(NamespacedIdentifier::new_assume_valid( + "Apples_id_count_idx_btree" + ))), "{steps:?}" ); - assert!(steps.contains(&AutoMigrateStep::ChangeAccess(&bananas)), "{steps:?}"); assert!( - steps.contains(&AutoMigrateStep::AddSequence(&bananas_sequence)), + steps.contains(&AutoMigrateStep::ChangeAccess(NamespacedIdentifier::new_assume_valid( + "Bananas" + ))), + "{steps:?}" + ); + assert!( + steps.contains(&AutoMigrateStep::AddSequence(NamespacedIdentifier::new_assume_valid( + "Bananas_id_seq" + ))), "{steps:?}" ); - assert!(steps.contains(&AutoMigrateStep::AddTable(&oranges)), "{steps:?}"); + assert!( + steps.contains(&AutoMigrateStep::AddTable(NamespacedIdentifier::new_assume_valid( + "Oranges" + ))), + "{steps:?}" + ); + // Schedule steps are keyed by TABLE name (schedules are 1:1 with tables). assert!( - steps.contains(&AutoMigrateStep::RemoveSchedule(&deliveries_schedule)), + steps.contains(&AutoMigrateStep::RemoveSchedule( + NamespacedIdentifier::new_assume_valid("Deliveries") + )), "{steps:?}" ); assert!( - steps.contains(&AutoMigrateStep::AddSchedule(&inspections_schedule)), + steps.contains(&AutoMigrateStep::AddSchedule(NamespacedIdentifier::new_assume_valid( + "Inspections" + ))), "{steps:?}" ); assert!( - steps.contains(&AutoMigrateStep::RemoveRowLevelSecurity(&sql_old.sql)), + steps.contains(&AutoMigrateStep::RemoveRowLevelSecurity(Box::from( + "SELECT * FROM Apples" + ))), "{steps:?}" ); assert!( - steps.contains(&AutoMigrateStep::AddRowLevelSecurity(&sql_new.sql)), + steps.contains(&AutoMigrateStep::AddRowLevelSecurity(Box::from( + "SELECT * FROM Bananas" + ))), "{steps:?}" ); - assert!(steps.contains(&AutoMigrateStep::ChangeColumns(&apples)), "{steps:?}"); assert!( - steps.contains(&AutoMigrateStep::ChangeColumns(&deliveries)), + steps.contains(&AutoMigrateStep::ChangeColumns(NamespacedIdentifier::new_assume_valid( + "Apples" + ))), + "{steps:?}" + ); + assert!( + steps.contains(&AutoMigrateStep::ChangeColumns(NamespacedIdentifier::new_assume_valid( + "Deliveries" + ))), "{steps:?}" ); assert!(steps.contains(&AutoMigrateStep::DisconnectAllUsers), "{steps:?}"); - assert!(steps.contains(&AutoMigrateStep::AddColumns(&bananas)), "{steps:?}"); + assert!( + steps.contains(&AutoMigrateStep::AddColumns(NamespacedIdentifier::new_assume_valid( + "Bananas" + ))), + "{steps:?}" + ); // Column is changed but it will not reflect in steps due to `AutoMigrateStep::AddColumns` - assert!(!steps.contains(&AutoMigrateStep::ChangeColumns(&bananas)), "{steps:?}"); + assert!( + !steps.contains(&AutoMigrateStep::ChangeColumns(NamespacedIdentifier::new_assume_valid( + "Bananas" + ))), + "{steps:?}" + ); - assert!(steps.contains(&AutoMigrateStep::RemoveView(&my_view)), "{steps:?}"); - assert!(steps.contains(&AutoMigrateStep::AddView(&my_view)), "{steps:?}"); + assert!( + steps.contains(&AutoMigrateStep::RemoveView(NamespacedIdentifier::new_assume_valid( + "my_view" + ))), + "{steps:?}" + ); + assert!( + steps.contains(&AutoMigrateStep::AddView(NamespacedIdentifier::new_assume_valid( + "my_view" + ))), + "{steps:?}" + ); } #[test] @@ -1846,14 +2035,18 @@ mod tests { ); }); - let my_view = expect_identifier("my_view"); - let plan = ponder_auto_migrate(&old_def, &new_def).expect("auto migration should succeed"); let steps = &plan.steps[..]; assert!(!plan.disconnects_all_users(), "{plan:#?}"); - assert!(steps.contains(&AutoMigrateStep::AddView(&my_view)), "{steps:?}"); - assert!(!steps.contains(&AutoMigrateStep::RemoveView(&my_view)), "{steps:?}"); + assert!( + steps.contains(&AutoMigrateStep::AddView(NamespacedIdentifier::new_assume_valid("my_view"))), + "{steps:?}" + ); + assert!( + !steps.contains(&AutoMigrateStep::RemoveView(NamespacedIdentifier::new_assume_valid("my_view"))), + "{steps:?}" + ); } #[test] @@ -1876,14 +2069,18 @@ mod tests { }); let new_def = create_module_def(|_| {}); - let my_view = expect_identifier("my_view"); - let plan = ponder_auto_migrate(&old_def, &new_def).expect("auto migration should succeed"); let steps = &plan.steps[..]; assert!(plan.disconnects_all_users(), "{plan:#?}"); - assert!(steps.contains(&AutoMigrateStep::RemoveView(&my_view)), "{steps:?}"); - assert!(!steps.contains(&AutoMigrateStep::AddView(&my_view)), "{steps:?}"); + assert!( + steps.contains(&AutoMigrateStep::RemoveView(NamespacedIdentifier::new_assume_valid("my_view"))), + "{steps:?}" + ); + assert!( + !steps.contains(&AutoMigrateStep::AddView(NamespacedIdentifier::new_assume_valid("my_view"))), + "{steps:?}" + ); } #[test] @@ -1970,23 +2167,23 @@ mod tests { }), }, ] { - let my_view = expect_identifier("my_view"); - - let plan = ponder_auto_migrate(&old_def, &new_def).expect("auto migration should succeed"); + let plan = ponder_auto_migrate(&old_def, &new_def).expect("auto migration should succeed"); let steps = &plan.steps[..]; assert!(!plan.disconnects_all_users(), "{name}, plan: {plan:#?}"); assert!( - steps.contains(&AutoMigrateStep::UpdateView(&my_view)), + steps.contains(&AutoMigrateStep::UpdateView(NamespacedIdentifier::new_assume_valid("my_view"))), "{name}, steps: {steps:?}" ); assert!( - !steps.contains(&AutoMigrateStep::AddView(&my_view)), + !steps.contains(&AutoMigrateStep::AddView(NamespacedIdentifier::new_assume_valid("my_view"))), "{name}, steps: {steps:?}" ); assert!( - !steps.contains(&AutoMigrateStep::RemoveView(&my_view)), + !steps.contains(&AutoMigrateStep::RemoveView(NamespacedIdentifier::new_assume_valid( + "my_view" + ))), "{name}, steps: {steps:?}" ); } @@ -2019,22 +2216,26 @@ mod tests { let old_def = module_def(); let new_def = module_def(); - let level_2_person = expect_identifier("Level2Person"); - let plan = ponder_auto_migrate(&old_def, &new_def).expect("auto migration should succeed"); let steps = &plan.steps[..]; assert!(!plan.disconnects_all_users(), "{plan:#?}"); assert!( - steps.contains(&AutoMigrateStep::UpdateView(&level_2_person)), + steps.contains(&AutoMigrateStep::UpdateView(NamespacedIdentifier::new_assume_valid( + "Level2Person" + ))), "steps: {steps:?}" ); assert!( - !steps.contains(&AutoMigrateStep::AddView(&level_2_person)), + !steps.contains(&AutoMigrateStep::AddView(NamespacedIdentifier::new_assume_valid( + "Level2Person" + ))), "steps: {steps:?}" ); assert!( - !steps.contains(&AutoMigrateStep::RemoveView(&level_2_person)), + !steps.contains(&AutoMigrateStep::RemoveView(NamespacedIdentifier::new_assume_valid( + "Level2Person" + ))), "steps: {steps:?}" ); } @@ -2368,23 +2569,25 @@ mod tests { }), }, ] { - let my_view = expect_identifier("my_view"); - - let plan = ponder_auto_migrate(&old_def, &new_def).expect("auto migration should succeed"); + let plan = ponder_auto_migrate(&old_def, &new_def).expect("auto migration should succeed"); let steps = &plan.steps[..]; assert!(plan.disconnects_all_users(), "{name}, plan: {plan:?}"); assert!( - steps.contains(&AutoMigrateStep::AddView(&my_view)), + steps.contains(&AutoMigrateStep::AddView(NamespacedIdentifier::new_assume_valid("my_view"))), "{name}, steps: {steps:?}" ); assert!( - steps.contains(&AutoMigrateStep::RemoveView(&my_view)), + steps.contains(&AutoMigrateStep::RemoveView(NamespacedIdentifier::new_assume_valid( + "my_view" + ))), "{name}, steps: {steps:?}" ); assert!( - !steps.contains(&AutoMigrateStep::UpdateView(&my_view)), + !steps.contains(&AutoMigrateStep::UpdateView(NamespacedIdentifier::new_assume_valid( + "my_view" + ))), "{name}, steps: {steps:?}" ); } @@ -2514,12 +2717,11 @@ mod tests { .finish(); }); - let drop_table = expect_identifier("Drop"); let plan = ponder_auto_migrate(&old, &new).expect("removing a table should produce a valid plan"); assert_eq!( plan.steps, &[ - AutoMigrateStep::RemoveTable(&drop_table), + AutoMigrateStep::RemoveTable(NamespacedIdentifier::new_assume_valid("Drop")), AutoMigrateStep::DisconnectAllUsers, ], ); @@ -2537,15 +2739,428 @@ mod tests { }); let new = create_module_def(|_builder| {}); - let drop_table = expect_identifier("Drop"); let plan = ponder_auto_migrate(&old, &new).expect("removing a table should produce a valid plan"); assert_eq!( plan.steps, &[ - AutoMigrateStep::RemoveTable(&drop_table), + AutoMigrateStep::RemoveTable(NamespacedIdentifier::new_assume_valid("Drop")), AutoMigrateStep::DisconnectAllUsers, ], "plan should only contain RemoveTable + DisconnectAllUsers, no orphan sub-object steps" ); } + + // ---- Mount helpers ---------------------------------------------------------- + + fn make_mount(namespace: &str, build: impl Fn(&mut RawModuleDefV10Builder)) -> RawModuleMountV10 { + let mut builder = RawModuleDefV10Builder::new(); + build(&mut builder); + RawModuleMountV10 { + namespace: namespace.to_string(), + module: builder.finish(), + } + } + + fn create_module_def_with_mounts( + build_root: impl Fn(&mut RawModuleDefV10Builder), + mounts: Vec, + ) -> ModuleDef { + let mut builder = RawModuleDefV10Builder::new(); + build_root(&mut builder); + let mut raw = builder.finish(); + if !mounts.is_empty() { + raw.sections.push(RawModuleDefV10Section::Mounts(mounts)); + } + raw.try_into().expect("should be a valid module definition") + } + + // ---- Mount auto-migration tests --------------------------------------------- + + #[test] + fn mounted_table_unchanged() { + let mount = || { + make_mount("lib", |b| { + b.build_table_with_new_type("sessions", ProductType::from([("id", AlgebraicType::U64)]), true) + .finish(); + }) + }; + let old = create_module_def_with_mounts(|_| {}, vec![mount()]); + let new = create_module_def_with_mounts(|_| {}, vec![mount()]); + + let plan = ponder_auto_migrate(&old, &new).expect("no-op migration should succeed"); + let namespaced: Vec<_> = plan + .steps + .iter() + .filter(|s| format!("{s:?}").contains("lib.sessions")) + .collect(); + assert!(namespaced.is_empty(), "unchanged mount should produce no steps for lib.sessions: {plan:#?}"); + } + + #[test] + fn mounted_add_table() { + let old = create_module_def_with_mounts( + |_| {}, + vec![make_mount("lib", |b| { + b.build_table_with_new_type("sessions", ProductType::from([("id", AlgebraicType::U64)]), true) + .finish(); + })], + ); + let new = create_module_def_with_mounts( + |_| {}, + vec![make_mount("lib", |b| { + b.build_table_with_new_type("sessions", ProductType::from([("id", AlgebraicType::U64)]), true) + .finish(); + b.build_table_with_new_type("tokens", ProductType::from([("id", AlgebraicType::U64)]), true) + .finish(); + })], + ); + + let plan = ponder_auto_migrate(&old, &new).expect("adding a mounted table should succeed"); + let steps = &plan.steps[..]; + + assert!(!plan.disconnects_all_users(), "{plan:#?}"); + assert!( + steps.contains(&AutoMigrateStep::AddTable(NamespacedIdentifier::new_assume_valid("lib.tokens"))), + "{steps:?}" + ); + } + + #[test] + fn mounted_remove_table() { + let old = create_module_def_with_mounts( + |_| {}, + vec![make_mount("lib", |b| { + b.build_table_with_new_type("sessions", ProductType::from([("id", AlgebraicType::U64)]), true) + .finish(); + b.build_table_with_new_type("tokens", ProductType::from([("id", AlgebraicType::U64)]), true) + .finish(); + })], + ); + let new = create_module_def_with_mounts( + |_| {}, + vec![make_mount("lib", |b| { + b.build_table_with_new_type("sessions", ProductType::from([("id", AlgebraicType::U64)]), true) + .finish(); + })], + ); + + let plan = ponder_auto_migrate(&old, &new).expect("removing a mounted table should succeed"); + let steps = &plan.steps[..]; + + assert!(plan.disconnects_all_users(), "{plan:#?}"); + assert!( + steps.contains(&AutoMigrateStep::RemoveTable(NamespacedIdentifier::new_assume_valid("lib.tokens"))), + "{steps:?}" + ); + } + + #[test] + fn mounted_add_index() { + let sessions_without_index = || { + make_mount("lib", |b| { + b.build_table_with_new_type("sessions", ProductType::from([("id", AlgebraicType::U64)]), true) + .finish(); + }) + }; + let sessions_with_index = || { + make_mount("lib", |b| { + b.build_table_with_new_type("sessions", ProductType::from([("id", AlgebraicType::U64)]), true) + .with_index(btree(0), "sessions_id_idx", "sessions_id_idx") + .finish(); + }) + }; + + let old = create_module_def_with_mounts(|_| {}, vec![sessions_without_index()]); + let new = create_module_def_with_mounts(|_| {}, vec![sessions_with_index()]); + + let plan = ponder_auto_migrate(&old, &new).expect("adding a mounted index should succeed"); + let steps = &plan.steps[..]; + + assert!( + steps.contains(&AutoMigrateStep::AddIndex(NamespacedIdentifier::new_assume_valid( + "lib.sessions_id_idx_btree" + ))), + "{steps:?}" + ); + } + + #[test] + fn mounted_remove_index() { + let sessions_without_index = || { + make_mount("lib", |b| { + b.build_table_with_new_type("sessions", ProductType::from([("id", AlgebraicType::U64)]), true) + .finish(); + }) + }; + let sessions_with_index = || { + make_mount("lib", |b| { + b.build_table_with_new_type("sessions", ProductType::from([("id", AlgebraicType::U64)]), true) + .with_index(btree(0), "sessions_id_idx", "sessions_id_idx") + .finish(); + }) + }; + + let old = create_module_def_with_mounts(|_| {}, vec![sessions_with_index()]); + let new = create_module_def_with_mounts(|_| {}, vec![sessions_without_index()]); + + let plan = ponder_auto_migrate(&old, &new).expect("removing a mounted index should succeed"); + let steps = &plan.steps[..]; + + assert!( + steps.contains(&AutoMigrateStep::RemoveIndex(NamespacedIdentifier::new_assume_valid( + "lib.sessions_id_idx_btree" + ))), + "{steps:?}" + ); + } + + #[test] + fn mounted_add_sequence() { + let without_seq = || { + make_mount("lib", |b| { + b.build_table_with_new_type("sessions", ProductType::from([("id", AlgebraicType::U64)]), true) + .finish(); + }) + }; + let with_seq = || { + make_mount("lib", |b| { + b.build_table_with_new_type("sessions", ProductType::from([("id", AlgebraicType::U64)]), true) + .with_column_sequence(0) + .finish(); + }) + }; + + let old = create_module_def_with_mounts(|_| {}, vec![without_seq()]); + let new = create_module_def_with_mounts(|_| {}, vec![with_seq()]); + + let plan = ponder_auto_migrate(&old, &new).expect("adding a mounted sequence should succeed"); + let steps = &plan.steps[..]; + + assert!( + steps.iter().any(|s| matches!( + s, + AutoMigrateStep::AddSequence(n) if n.starts_with("lib.sessions") + )), + "expected AddSequence for lib.sessions_*: {steps:?}" + ); + assert!( + plan.prechecks.iter().any(|p| matches!( + p, + AutoMigratePrecheck::CheckAddSequenceRangeValid(n) if n.starts_with("lib.sessions") + )), + "expected CheckAddSequenceRangeValid precheck: {:?}", + plan.prechecks + ); + } + + #[test] + fn mounted_add_view() { + let without_view = || make_mount("lib", |_| {}); + let with_view = || { + make_mount("lib", |b| { + let ret_ref = b.add_algebraic_type( + [], + "lib_view_return", + AlgebraicType::product([("a", AlgebraicType::U64)]), + true, + ); + b.add_view( + "lib_view", + 0, + true, + true, + ProductType::from([("x", AlgebraicType::U32)]), + AlgebraicType::array(AlgebraicType::Ref(ret_ref)), + ); + }) + }; + + let old = create_module_def_with_mounts(|_| {}, vec![without_view()]); + let new = create_module_def_with_mounts(|_| {}, vec![with_view()]); + + let plan = ponder_auto_migrate(&old, &new).expect("adding a mounted view should succeed"); + let steps = &plan.steps[..]; + + assert!(!plan.disconnects_all_users(), "{plan:#?}"); + assert!( + steps.contains(&AutoMigrateStep::AddView(NamespacedIdentifier::new_assume_valid("lib.lib_view"))), + "{steps:?}" + ); + } + + #[test] + fn mounted_remove_view() { + let without_view = || make_mount("lib", |_| {}); + let with_view = || { + make_mount("lib", |b| { + let ret_ref = b.add_algebraic_type( + [], + "lib_view_return", + AlgebraicType::product([("a", AlgebraicType::U64)]), + true, + ); + b.add_view( + "lib_view", + 0, + true, + true, + ProductType::from([("x", AlgebraicType::U32)]), + AlgebraicType::array(AlgebraicType::Ref(ret_ref)), + ); + }) + }; + + let old = create_module_def_with_mounts(|_| {}, vec![with_view()]); + let new = create_module_def_with_mounts(|_| {}, vec![without_view()]); + + let plan = ponder_auto_migrate(&old, &new).expect("removing a mounted view should succeed"); + let steps = &plan.steps[..]; + + assert!(plan.disconnects_all_users(), "{plan:#?}"); + assert!( + steps.contains(&AutoMigrateStep::RemoveView(NamespacedIdentifier::new_assume_valid( + "lib.lib_view" + ))), + "{steps:?}" + ); + } + + #[test] + fn add_whole_mount() { + let old = create_module_def_with_mounts(|_| {}, vec![]); + let new = create_module_def_with_mounts( + |_| {}, + vec![make_mount("lib", |b| { + b.build_table_with_new_type("sessions", ProductType::from([("id", AlgebraicType::U64)]), true) + .finish(); + b.build_table_with_new_type("tokens", ProductType::from([("id", AlgebraicType::U64)]), true) + .finish(); + })], + ); + + let plan = ponder_auto_migrate(&old, &new).expect("adding a whole mount should succeed"); + let steps = &plan.steps[..]; + + assert!(!plan.disconnects_all_users(), "{plan:#?}"); + assert!( + steps.contains(&AutoMigrateStep::AddTable(NamespacedIdentifier::new_assume_valid("lib.sessions"))), + "{steps:?}" + ); + assert!( + steps.contains(&AutoMigrateStep::AddTable(NamespacedIdentifier::new_assume_valid("lib.tokens"))), + "{steps:?}" + ); + } + + #[test] + fn remove_whole_mount() { + let old = create_module_def_with_mounts( + |_| {}, + vec![make_mount("lib", |b| { + b.build_table_with_new_type("sessions", ProductType::from([("id", AlgebraicType::U64)]), true) + .finish(); + b.build_table_with_new_type("tokens", ProductType::from([("id", AlgebraicType::U64)]), true) + .finish(); + })], + ); + let new = create_module_def_with_mounts(|_| {}, vec![]); + + let plan = ponder_auto_migrate(&old, &new).expect("removing a whole mount should succeed"); + let steps = &plan.steps[..]; + + assert!(plan.disconnects_all_users(), "{plan:#?}"); + assert!( + steps.contains(&AutoMigrateStep::RemoveTable(NamespacedIdentifier::new_assume_valid( + "lib.sessions" + ))), + "{steps:?}" + ); + assert!( + steps.contains(&AutoMigrateStep::RemoveTable(NamespacedIdentifier::new_assume_valid( + "lib.tokens" + ))), + "{steps:?}" + ); + } + + #[test] + fn nested_mounted_add_table() { + let make_nested_def = |add_baz_items: bool| { + let baz_mount = make_mount("baz", |b| { + b.build_table_with_new_type("sessions", ProductType::from([("id", AlgebraicType::U64)]), true) + .finish(); + if add_baz_items { + b.build_table_with_new_type( + "baz_items", + ProductType::from([("id", AlgebraicType::U32)]), + true, + ) + .finish(); + } + }); + + let mut auth_builder = RawModuleDefV10Builder::new(); + auth_builder + .build_table_with_new_type("auth_users", ProductType::from([("id", AlgebraicType::U64)]), true) + .finish(); + let mut auth_raw = auth_builder.finish(); + auth_raw + .sections + .push(RawModuleDefV10Section::Mounts(vec![baz_mount])); + + let auth_mount = RawModuleMountV10 { + namespace: "auth".to_string(), + module: auth_raw, + }; + + let root_builder = RawModuleDefV10Builder::new(); + let mut root_raw = root_builder.finish(); + root_raw + .sections + .push(RawModuleDefV10Section::Mounts(vec![auth_mount])); + root_raw + .try_into() + .expect("should be a valid module definition") + }; + + let old: ModuleDef = make_nested_def(false); + let new: ModuleDef = make_nested_def(true); + + let plan = ponder_auto_migrate(&old, &new).expect("adding a deeply nested table should succeed"); + let steps = &plan.steps[..]; + + assert!(!plan.disconnects_all_users(), "{plan:#?}"); + assert!( + steps.contains(&AutoMigrateStep::AddTable(NamespacedIdentifier::new_assume_valid( + "auth.baz.baz_items" + ))), + "{steps:?}" + ); + } + + #[test] + fn mounted_remove_table_no_orphan_sub_objects() { + let old = create_module_def_with_mounts( + |_| {}, + vec![make_mount("lib", |b| { + b.build_table_with_new_type("sessions", ProductType::from([("id", AlgebraicType::U64)]), true) + .with_primary_key(0) + .with_unique_constraint(0) + .with_index(btree(0), "sessions_id_idx", "sessions_id_idx") + .finish(); + })], + ); + let new = create_module_def_with_mounts(|_| {}, vec![make_mount("lib", |_| {})]); + + let plan = + ponder_auto_migrate(&old, &new).expect("removing a mounted table with sub-objects should succeed"); + assert_eq!( + plan.steps, + &[ + AutoMigrateStep::RemoveTable(NamespacedIdentifier::new_assume_valid("lib.sessions")), + AutoMigrateStep::DisconnectAllUsers, + ], + "should only contain RemoveTable + DisconnectAllUsers, no orphan sub-object steps" + ); + } } diff --git a/crates/schema/src/auto_migrate/formatter.rs b/crates/schema/src/auto_migrate/formatter.rs index 1f7377209bf..9c4c6df05ba 100644 --- a/crates/schema/src/auto_migrate/formatter.rs +++ b/crates/schema/src/auto_migrate/formatter.rs @@ -2,14 +2,14 @@ use std::io; -use super::{AutoMigratePlan, ModuleDefLookup, TableDef}; +use super::AutoMigratePlan; use crate::{ auto_migrate::AutoMigrateStep, - def::{ConstraintData, FunctionKind, ModuleDef, ScheduleDef, ViewDef}, + def::{ConstraintData, FunctionKind, ModuleDef}, identifier::Identifier, }; use itertools::Itertools; -use spacetimedb_lib::db::raw_def::v9::{RawRowLevelSecurityDefV9, TableAccess, TableType}; +use spacetimedb_lib::db::raw_def::v9::{TableAccess, TableType}; use spacetimedb_primitives::ColId; use spacetimedb_sats::raw_identifier::RawIdentifier; use spacetimedb_sats::{AlgebraicType, AlgebraicValue, WithTypespace}; @@ -32,80 +32,83 @@ fn format_step( ) -> Result<(), FormattingErrors> { match step { AutoMigrateStep::AddView(view) => { - let view_info = extract_view_info(*view, plan.new)?; + let view_info = extract_view_info(&**view, plan.new)?; f.format_view(&view_info, Action::Created) } AutoMigrateStep::RemoveView(view) => { - let view_info = extract_view_info(*view, plan.old)?; + let view_info = extract_view_info(&**view, plan.old)?; f.format_view(&view_info, Action::Removed) } // This means the body of the view may have been updated. // So we must recompute it and send any updates to clients. // No need to include this step in the formatted plan. AutoMigrateStep::UpdateView(_) => Ok(()), - AutoMigrateStep::RemoveTable(t) => { - let table_def: &TableDef = plan.old.expect_lookup(*t); - f.format_remove_table(&table_def.name) - } + AutoMigrateStep::RemoveTable(t) => f.format_remove_table(&**t), AutoMigrateStep::AddTable(t) => { - let table_info = extract_table_info(*t, plan)?; + let table_info = extract_table_info(&**t, plan)?; f.format_add_table(&table_info) } AutoMigrateStep::AddIndex(index) => { - let index_info = extract_index_info(*index, plan.new)?; + let index_info = extract_index_info(&**index, plan.new)?; f.format_index(&index_info, Action::Created) } AutoMigrateStep::RemoveIndex(index) => { - let index_info = extract_index_info(*index, plan.old)?; + let index_info = extract_index_info(&**index, plan.old)?; f.format_index(&index_info, Action::Removed) } AutoMigrateStep::RemoveConstraint(constraint) => { - let constraint_info = extract_constraint_info(*constraint, plan.old)?; + let constraint_info = extract_constraint_info(&**constraint, plan.old)?; f.format_constraint(&constraint_info, Action::Removed) } AutoMigrateStep::AddSequence(sequence) => { - let sequence_info = extract_sequence_info(*sequence, plan.new)?; + let sequence_info = extract_sequence_info(&**sequence, plan.new)?; f.format_sequence(&sequence_info, Action::Created) } AutoMigrateStep::RemoveSequence(sequence) => { - let sequence_info = extract_sequence_info(*sequence, plan.old)?; + let sequence_info = extract_sequence_info(&**sequence, plan.old)?; f.format_sequence(&sequence_info, Action::Removed) } AutoMigrateStep::ChangeAccess(table) => { - let access_info = extract_access_change_info(*table, plan)?; + let access_info = extract_access_change_info(&**table, plan)?; f.format_change_access(&access_info) } AutoMigrateStep::ChangePrimaryKey(table) => { - let old_table = plan.old.lookup_expect::(*table); - let new_table = plan.new.lookup_expect::(*table); - f.format_change_primary_key(table, old_table.primary_key, new_table.primary_key) + let (_, _, old_table) = plan + .old + .find_table_by_full_name(&**table) + .ok_or_else(|| FormattingErrors::TableNotFound { table: (&**table).into() })?; + let (_, _, new_table) = plan + .new + .find_table_by_full_name(&**table) + .ok_or_else(|| FormattingErrors::TableNotFound { table: (&**table).into() })?; + f.format_change_primary_key(&**table, old_table.primary_key, new_table.primary_key) } AutoMigrateStep::AddSchedule(schedule) => { - let schedule_info = extract_schedule_info(*schedule, plan.new)?; + let schedule_info = extract_schedule_info(&**schedule, plan.new)?; f.format_schedule(&schedule_info, Action::Created) } AutoMigrateStep::RemoveSchedule(schedule) => { - let schedule_info = extract_schedule_info(*schedule, plan.old)?; + let schedule_info = extract_schedule_info(&**schedule, plan.old)?; f.format_schedule(&schedule_info, Action::Removed) } AutoMigrateStep::AddRowLevelSecurity(rls) => { - if let Some(rls_info) = extract_rls_info(*rls, plan)? { + if let Some(rls_info) = extract_rls_info(rls.as_ref(), plan)? { f.format_rls(&rls_info, Action::Created)?; } Ok(()) } AutoMigrateStep::RemoveRowLevelSecurity(rls) => { - if let Some(rls_info) = extract_rls_info(*rls, plan)? { + if let Some(rls_info) = extract_rls_info(rls.as_ref(), plan)? { f.format_rls(&rls_info, Action::Removed)?; } Ok(()) } AutoMigrateStep::ChangeColumns(table) => { - let column_changes = extract_column_changes(*table, plan)?; + let column_changes = extract_column_changes(&**table, plan)?; f.format_change_columns(&column_changes) } AutoMigrateStep::AddColumns(table) => { - let new_columns = extract_new_columns(*table, plan)?; + let new_columns = extract_new_columns(&**table, plan)?; f.format_add_columns(&new_columns) } AutoMigrateStep::DisconnectAllUsers => f.format_disconnect_warning(), @@ -150,7 +153,7 @@ pub enum Action { pub trait MigrationFormatter { fn format_header(&mut self) -> io::Result<()>; fn format_add_table(&mut self, table_info: &TableInfo) -> io::Result<()>; - fn format_remove_table(&mut self, table_name: &Identifier) -> io::Result<()>; + fn format_remove_table(&mut self, table_name: &str) -> io::Result<()>; fn format_view(&mut self, view_info: &ViewInfo, action: Action) -> io::Result<()>; fn format_index(&mut self, index_info: &IndexInfo, action: Action) -> io::Result<()>; fn format_constraint(&mut self, constraint_info: &ConstraintInfo, action: Action) -> io::Result<()>; @@ -212,7 +215,7 @@ pub struct ColumnInfo { pub struct ConstraintInfo { pub name: RawIdentifier, pub columns: Vec, - pub table_name: Identifier, + pub table_name: RawIdentifier, } #[derive(Debug, Clone, PartialEq)] @@ -226,18 +229,18 @@ pub struct IndexInfo { pub struct SequenceInfo { pub name: RawIdentifier, pub column_name: Identifier, - pub table_name: Identifier, + pub table_name: RawIdentifier, } #[derive(Debug, Clone, PartialEq)] pub struct AccessChangeInfo { - pub table_name: Identifier, + pub table_name: RawIdentifier, pub new_access: TableAccess, } #[derive(Debug, Clone, PartialEq)] pub struct ScheduleInfo { - pub table_name: Identifier, + pub table_name: RawIdentifier, pub function_name: Identifier, pub function_kind: FunctionKind, } @@ -249,7 +252,7 @@ pub struct RlsInfo { #[derive(Debug, Clone, PartialEq)] pub struct ColumnChanges { - pub table_name: Identifier, + pub table_name: RawIdentifier, pub changes: Vec, } @@ -268,24 +271,24 @@ pub enum ColumnChange { #[derive(Debug, Clone, PartialEq)] pub struct NewColumns { - pub table_name: Identifier, + pub table_name: RawIdentifier, pub columns: Vec, } -// Data extraction functions (these replace the original print functions' data gathering logic) fn extract_table_info( - table: ::Key<'_>, + full_name: &str, plan: &super::AutoMigratePlan, ) -> Result { - let table_def = plan.new.table(table).ok_or_else(|| FormattingErrors::TableNotFound { - table: table.to_string().into(), - })?; + let (_, owning_def, table_def) = + plan.new.find_table_by_full_name(full_name).ok_or_else(|| FormattingErrors::TableNotFound { + table: full_name.into(), + })?; let columns = table_def .columns .iter() .map(|column| { - let type_name = WithTypespace::new(plan.new.typespace(), &column.ty) + let type_name = WithTypespace::new(owning_def.typespace(), &column.ty) .resolve_refs() .map_err(|_| FormattingErrors::TypeResolution)?; Ok(ColumnInfo { @@ -312,7 +315,7 @@ fn extract_table_info( Ok(column.name.clone()) }) .collect::, FormattingErrors>>()?, - table_name: table_def.name.clone(), + table_name: RawIdentifier::new(full_name), }) }) .collect::, FormattingErrors>>()?; @@ -335,7 +338,7 @@ fn extract_table_info( Ok(IndexInfo { name: index.name.clone(), columns, - table_name: table_def.name.clone().into(), + table_name: RawIdentifier::new(full_name), }) }) .collect::, FormattingErrors>>()?; @@ -351,19 +354,19 @@ fn extract_table_info( Ok(SequenceInfo { name: sequence.name.clone(), column_name: column.name.clone(), - table_name: table_def.name.clone(), + table_name: RawIdentifier::new(full_name), }) }) .collect::, FormattingErrors>>()?; let schedule = table_def.schedule.as_ref().map(|schedule| ScheduleInfo { - table_name: table_def.name.clone(), + table_name: RawIdentifier::new(full_name), function_name: schedule.function_name.clone(), function_kind: schedule.function_kind, }); Ok(TableInfo { - name: table_def.name.clone().into(), + name: RawIdentifier::new(full_name), is_system: table_def.table_type == TableType::System, access: table_def.table_access, columns, @@ -374,22 +377,20 @@ fn extract_table_info( }) } -fn extract_view_info( - view: ::Key<'_>, - module_def: &ModuleDef, -) -> Result { - let view_def = module_def.view(view).ok_or_else(|| FormattingErrors::ViewNotFound { - view: view.to_string().into(), - })?; +fn extract_view_info(full_name: &str, module_def: &ModuleDef) -> Result { + let (_, owning_def, view_def) = + module_def.find_view_by_full_name(full_name).ok_or_else(|| FormattingErrors::ViewNotFound { + view: full_name.into(), + })?; - let name = view_def.name.clone().into(); + let name = RawIdentifier::new(full_name); let is_anonymous = view_def.is_anonymous; let params = view_def .param_columns .iter() .map(|column| { - let type_name = WithTypespace::new(module_def.typespace(), &column.ty) + let type_name = WithTypespace::new(owning_def.typespace(), &column.ty) .resolve_refs() .map_err(|_| FormattingErrors::TypeResolution)?; Ok(ViewParamInfo { @@ -403,7 +404,7 @@ fn extract_view_info( .return_columns .iter() .map(|column| { - let type_name = WithTypespace::new(module_def.typespace(), &column.ty) + let type_name = WithTypespace::new(owning_def.typespace(), &column.ty) .resolve_refs() .map_err(|_| FormattingErrors::TypeResolution)?; Ok(ViewColumnInfo { @@ -421,14 +422,9 @@ fn extract_view_info( }) } -fn extract_index_info( - index: ::Key<'_>, - module_def: &ModuleDef, -) -> Result { - let table_def = module_def - .stored_in_table_def(index) - .ok_or(FormattingErrors::IndexNotFound)?; - let index_def = table_def.indexes.get(index).ok_or(FormattingErrors::IndexNotFound)?; +fn extract_index_info(full_name: &str, module_def: &ModuleDef) -> Result { + let (prefix, _, table_def, index_def) = + module_def.find_index_by_full_name(full_name).ok_or(FormattingErrors::IndexNotFound)?; let columns = index_def .algorithm @@ -441,22 +437,15 @@ fn extract_index_info( .collect::, FormattingErrors>>()?; Ok(IndexInfo { - name: index_def.name.clone(), + name: RawIdentifier::new(full_name), columns, - table_name: table_def.name.clone().into(), + table_name: RawIdentifier::new(format!("{}{}", prefix, &*table_def.accessor_name)), }) } -fn extract_constraint_info( - constraint: ::Key<'_>, - module_def: &ModuleDef, -) -> Result { - let table_def = module_def - .stored_in_table_def(constraint) - .ok_or(FormattingErrors::ConstraintNotFound)?; - let constraint_def = table_def - .constraints - .get(constraint) +fn extract_constraint_info(full_name: &str, module_def: &ModuleDef) -> Result { + let (prefix, _, table_def, constraint_def) = module_def + .find_constraint_by_full_name(full_name) .ok_or(FormattingErrors::ConstraintNotFound)?; let ConstraintData::Unique(unique_constraint_data) = &constraint_def.data; @@ -470,22 +459,15 @@ fn extract_constraint_info( .collect::, FormattingErrors>>()?; Ok(ConstraintInfo { - name: constraint_def.name.clone(), + name: RawIdentifier::new(full_name), columns, - table_name: table_def.name.clone(), + table_name: RawIdentifier::new(format!("{}{}", prefix, &*table_def.accessor_name)), }) } -fn extract_sequence_info( - sequence: ::Key<'_>, - module_def: &ModuleDef, -) -> Result { - let table_def = module_def - .stored_in_table_def(sequence) - .ok_or(FormattingErrors::SequenceNotFound)?; - let sequence_def = table_def - .sequences - .get(sequence) +fn extract_sequence_info(full_name: &str, module_def: &ModuleDef) -> Result { + let (prefix, _, table_def, sequence_def) = module_def + .find_sequence_by_full_name(full_name) .ok_or(FormattingErrors::SequenceNotFound)?; let column = table_def @@ -493,47 +475,45 @@ fn extract_sequence_info( .ok_or(FormattingErrors::ColumnNotFound)?; Ok(SequenceInfo { - name: sequence_def.name.clone(), + name: RawIdentifier::new(full_name), column_name: column.name.clone(), - table_name: table_def.name.clone(), + table_name: RawIdentifier::new(format!("{}{}", prefix, &*table_def.accessor_name)), }) } fn extract_access_change_info( - table: ::Key<'_>, + full_name: &str, plan: &super::AutoMigratePlan, ) -> Result { - let table_def = plan.new.table(table).ok_or_else(|| FormattingErrors::TableNotFound { - table: table.to_string().into(), - })?; + let (_, _, table_def) = + plan.new.find_table_by_full_name(full_name).ok_or_else(|| FormattingErrors::TableNotFound { + table: full_name.into(), + })?; Ok(AccessChangeInfo { - table_name: table_def.name.clone(), + table_name: RawIdentifier::new(full_name), new_access: table_def.table_access, }) } -fn extract_schedule_info( - schedule_table: ::Key<'_>, - module_def: &ModuleDef, -) -> Result { - let schedule_def: &ScheduleDef = module_def - .lookup(schedule_table) +fn extract_schedule_info(table_full_name: &str, module_def: &ModuleDef) -> Result { + let (_, _, table_def) = module_def + .find_table_by_full_name(table_full_name) .ok_or(FormattingErrors::ScheduleNotFound)?; + let schedule_def = table_def.schedule.as_ref().ok_or(FormattingErrors::ScheduleNotFound)?; Ok(ScheduleInfo { - table_name: schedule_def.name.clone(), + table_name: RawIdentifier::new(table_full_name), function_name: schedule_def.function_name.clone(), function_kind: schedule_def.function_kind, }) } -fn extract_rls_info( - rls: ::Key<'_>, - plan: &super::AutoMigratePlan, -) -> Result, FormattingErrors> { - // Skip if policy unchanged (implementation detail workaround) - if plan.old.lookup::(rls) == plan.new.lookup::(rls) { +fn extract_rls_info(rls: &str, plan: &super::AutoMigratePlan) -> Result, FormattingErrors> { + let in_old = plan.old.row_level_security().any(|r| &*r.sql == rls); + let in_new = plan.new.row_level_security().any(|r| &*r.sql == rls); + // Skip if policy is unchanged (present in both old and new). + if in_old == in_new { return Ok(None); } @@ -543,15 +523,17 @@ fn extract_rls_info( } fn extract_column_changes( - table: ::Key<'_>, + full_name: &str, plan: &super::AutoMigratePlan, ) -> Result { - let old_table = plan.old.table(table).ok_or_else(|| FormattingErrors::TableNotFound { - table: table.to_string().into(), - })?; - let new_table = plan.new.table(table).ok_or_else(|| FormattingErrors::TableNotFound { - table: table.to_string().into(), - })?; + let (_, old_owning, old_table) = + plan.old.find_table_by_full_name(full_name).ok_or_else(|| FormattingErrors::TableNotFound { + table: full_name.into(), + })?; + let (_, new_owning, new_table) = + plan.new.find_table_by_full_name(full_name).ok_or_else(|| FormattingErrors::TableNotFound { + table: full_name.into(), + })?; let mut changes = Vec::new(); @@ -565,10 +547,10 @@ fn extract_column_changes( }); } if old_col.ty != new_col.ty { - let old_type = WithTypespace::new(plan.old.typespace(), &old_col.ty) + let old_type = WithTypespace::new(old_owning.typespace(), &old_col.ty) .resolve_refs() .map_err(|_| FormattingErrors::TypeResolution)?; - let new_type = WithTypespace::new(plan.new.typespace(), &new_col.ty) + let new_type = WithTypespace::new(new_owning.typespace(), &new_col.ty) .resolve_refs() .map_err(|_| FormattingErrors::TypeResolution)?; changes.push(ColumnChange::TypeChanged { @@ -581,26 +563,25 @@ fn extract_column_changes( } Ok(ColumnChanges { - table_name: new_table.name.clone(), + table_name: RawIdentifier::new(full_name), changes, }) } -fn extract_new_columns( - table: ::Key<'_>, - plan: &super::AutoMigratePlan, -) -> Result { - let table_def = plan.new.table(table).ok_or_else(|| FormattingErrors::TableNotFound { - table: table.to_string().into(), - })?; - let old_table_def = plan.old.table(table).ok_or_else(|| FormattingErrors::TableNotFound { - table: table.to_string().into(), - })?; +fn extract_new_columns(full_name: &str, plan: &super::AutoMigratePlan) -> Result { + let (_, new_owning, table_def) = + plan.new.find_table_by_full_name(full_name).ok_or_else(|| FormattingErrors::TableNotFound { + table: full_name.into(), + })?; + let (_, _, old_table_def) = + plan.old.find_table_by_full_name(full_name).ok_or_else(|| FormattingErrors::TableNotFound { + table: full_name.into(), + })?; let mut new_columns = Vec::new(); for column in &table_def.columns { if !old_table_def.columns.iter().any(|c| c.col_id == column.col_id) { - let type_name = WithTypespace::new(plan.new.typespace(), &column.ty) + let type_name = WithTypespace::new(new_owning.typespace(), &column.ty) .resolve_refs() .map_err(|_| FormattingErrors::TypeResolution)?; new_columns.push(ColumnInfo { @@ -612,7 +593,7 @@ fn extract_new_columns( } Ok(NewColumns { - table_name: table_def.name.clone(), + table_name: RawIdentifier::new(full_name), columns: new_columns, }) } diff --git a/crates/schema/src/auto_migrate/termcolor_formatter.rs b/crates/schema/src/auto_migrate/termcolor_formatter.rs index 31807d14b30..4b3393eb50b 100644 --- a/crates/schema/src/auto_migrate/termcolor_formatter.rs +++ b/crates/schema/src/auto_migrate/termcolor_formatter.rs @@ -7,7 +7,6 @@ use spacetimedb_sats::algebraic_type::fmt::fmt_algebraic_type; use termcolor::{Buffer, Color, ColorChoice, ColorSpec, WriteColor}; use crate::auto_migrate::formatter::ViewInfo; -use crate::identifier::Identifier; use super::formatter::{ AccessChangeInfo, Action, ColumnChange, ColumnChanges, ConstraintInfo, IndexInfo, MigrationFormatter, NewColumns, @@ -231,7 +230,7 @@ impl MigrationFormatter for TermColorFormatter { self.write_line("") } - fn format_remove_table(&mut self, table_name: &Identifier) -> io::Result<()> { + fn format_remove_table(&mut self, table_name: &str) -> io::Result<()> { self.write_action_prefix(&Action::Removed)?; self.buffer.write_all(b" table: ")?; self.write_colored(table_name, Some(self.colors.table_name), true)?; diff --git a/crates/schema/src/identifier.rs b/crates/schema/src/identifier.rs index 06a396ec012..3206e8e1a40 100644 --- a/crates/schema/src/identifier.rs +++ b/crates/schema/src/identifier.rs @@ -128,6 +128,78 @@ impl From for RawIdentifier { } } +/// An identifier that allows dot or slash separators between otherwise-normal identifier segments. +/// +/// Used for fully-qualified names of mounted module items, e.g.: +/// - `"lib.library_table"` (table name) +/// - `"lib.library_table_id_idx_btree"` (index name) +/// - `"lib/library_reducer"` (reducer name) +/// +/// Root-level items use their plain name with no separator (e.g., `"user"`). +/// Construction from known-valid components should use `new_assume_valid`. +#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct NamespacedIdentifier(Box); + +impl NamespacedIdentifier { + /// Construct without validation. Use when building from known-valid `Identifier` segments + /// (e.g., `format!("{}{}", prefix, &*identifier)`). + pub fn new_assume_valid(s: impl Into>) -> Self { + Self(s.into()) + } + + /// Validated construction: each segment (split on `.` and `/`) must satisfy XID rules. + pub fn new(s: impl Into>) -> Result { + let s = s.into(); + for segment in s.split(['.', '/']) { + Identifier::new(RawIdentifier::new(segment))?; + } + Ok(Self(s)) + } +} + +impl std::fmt::Debug for NamespacedIdentifier { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{:?}", &*self.0) + } +} + +impl std::fmt::Display for NamespacedIdentifier { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.0) + } +} + +impl std::ops::Deref for NamespacedIdentifier { + type Target = str; + fn deref(&self) -> &str { + &self.0 + } +} + +impl AsRef for NamespacedIdentifier { + fn as_ref(&self) -> &str { + &self.0 + } +} + +impl From for Box { + fn from(id: NamespacedIdentifier) -> Self { + id.0 + } +} + +impl From<&str> for NamespacedIdentifier { + fn from(s: &str) -> Self { + Self::new_assume_valid(s) + } +} + +impl From for NamespacedIdentifier { + fn from(s: String) -> Self { + Self::new_assume_valid(s) + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/schema/src/schema.rs b/crates/schema/src/schema.rs index e62b9dc6963..4492f5b83d8 100644 --- a/crates/schema/src/schema.rs +++ b/crates/schema/src/schema.rs @@ -1049,7 +1049,10 @@ impl Schema for TableSchema { } fn check_compatible(&self, module_def: &ModuleDef, def: &Self::Def) -> Result<(), anyhow::Error> { - ensure_eq!(&self.table_name[..], &def.name[..], "Table name mismatch"); + // Mounted tables are stored in the DB with a namespace prefix (e.g. "lib.library_table"), + // but the def's name is just the local name ("library_table"). Strip any prefix before comparing. + let self_local_name = self.table_name.rsplit('.').next().unwrap_or(&self.table_name[..]); + ensure_eq!(self_local_name, &def.name[..], "Table name mismatch"); ensure_eq!(self.primary_key, def.primary_key, "Primary key mismatch"); let def_table_access: StAccess = (def.table_access).into(); ensure_eq!(self.table_access, def_table_access, "Table access mismatch"); @@ -1065,10 +1068,15 @@ impl Schema for TableSchema { } ensure_eq!(self.columns.len(), def.columns.len(), "Column count mismatch"); + // Index names in the DB are prefixed for mounted tables (e.g. "lib.library_table_id_idx_btree"), + // but def.indexes is keyed by the bare name. Derive the namespace prefix length from the + // difference between the full DB table name and the def's canonical (local) table name. + let ns_prefix_len = self.table_name.len() - def.name.len(); for index in &self.indexes { + let bare_name = &index.index_name[ns_prefix_len..]; let index_def = def .indexes - .get(&index.index_name) + .get(bare_name) .ok_or_else(|| anyhow::anyhow!("Index {} not found in definition", index.index_id.0))?; index.check_compatible(module_def, index_def)?; } @@ -1418,8 +1426,11 @@ impl Schema for ScheduleSchema { fn check_compatible(&self, _module_def: &ModuleDef, def: &Self::Def) -> Result<(), anyhow::Error> { ensure_eq!(&self.schedule_name[..], &def.name[..], "Schedule name mismatch"); + // For mounted tables, schedule function names in the DB are namespace-prefixed using '/' + // (e.g. "lib/library_scheduled_procedure") while def.function_name is the bare name. + let ns_len = self.function_name.len().saturating_sub(def.function_name.len()); ensure_eq!( - &self.function_name[..], + &self.function_name[ns_len..], &def.function_name[..], "Schedule function name mismatch" ); @@ -1487,7 +1498,11 @@ impl Schema for IndexSchema { } fn check_compatible(&self, _module_def: &ModuleDef, def: &Self::Def) -> Result<(), anyhow::Error> { - ensure_eq!(&self.index_name[..], &def.name[..], "Index name mismatch"); + // For mounted tables, the DB stores index names with a namespace prefix + // (e.g. "lib.library_table_id_idx_btree") while def.name is the bare name. + // Strip any prefix by comparing only the suffix of length def.name.len(). + let ns_len = self.index_name.len().saturating_sub(def.name.len()); + ensure_eq!(&self.index_name[ns_len..], &def.name[..], "Index name mismatch"); ensure_eq!(&self.index_algorithm, &def.algorithm, "Index algorithm mismatch"); Ok(()) } diff --git a/crates/schema/src/snapshots/spacetimedb_schema__auto_migrate__tests__updated pretty print no color.snap b/crates/schema/src/snapshots/spacetimedb_schema__auto_migrate__tests__updated pretty print no color.snap index bb6cbc98ef0..dea71a64749 100644 --- a/crates/schema/src/snapshots/spacetimedb_schema__auto_migrate__tests__updated pretty print no color.snap +++ b/crates/schema/src/snapshots/spacetimedb_schema__auto_migrate__tests__updated pretty print no color.snap @@ -9,7 +9,7 @@ Database Migration Plan ▸ Removed index Apples_id_name_idx_btree on [id, name] of table Apples ▸ Removed unique constraint Apples_id_key on [id] of table Apples ▸ Removed auto-increment constraint Apples_id_seq on column id of table Apples -▸ Removed schedule for table Deliveries_sched calling reducer check_deliveries +▸ Removed schedule for table Deliveries calling reducer check_deliveries ▸ ▸ Removed anonymous view: my_view Parameters: • x: U32 @@ -38,7 +38,7 @@ Database Migration Plan ▸ Created index Apples_id_count_idx_btree on [id, count] of table Apples ▸ Created auto-increment constraint Bananas_id_seq on column id of table Bananas -▸ Created schedule for table Inspections_sched calling reducer perform_inspection +▸ Created schedule for table Inspections calling reducer perform_inspection ▸ ▸ Created anonymous view: my_view Parameters: • x: U32 diff --git a/crates/schema/src/snapshots/spacetimedb_schema__auto_migrate__tests__updated pretty print.snap b/crates/schema/src/snapshots/spacetimedb_schema__auto_migrate__tests__updated pretty print.snap index b9a9b7827bd..6d643270705 100644 --- a/crates/schema/src/snapshots/spacetimedb_schema__auto_migrate__tests__updated pretty print.snap +++ b/crates/schema/src/snapshots/spacetimedb_schema__auto_migrate__tests__updated pretty print.snap @@ -9,7 +9,7 @@ expression: "plan.pretty_print(PrettyPrintStyle::AnsiColor).expect(\"should pret ▸ Removed index Apples_id_name_idx_btree on [id, name] of table Apples ▸ Removed unique constraint Apples_id_key on [id] of table Apples ▸ Removed auto-increment constraint Apples_id_seq on column id of table Apples -▸ Removed schedule for table Deliveries_sched calling reducer check_deliveries +▸ Removed schedule for table Deliveries calling reducer check_deliveries ▸ ▸ Removed anonymous view: my_view Parameters: • x: U32 @@ -38,7 +38,7 @@ expression: "plan.pretty_print(PrettyPrintStyle::AnsiColor).expect(\"should pret ▸ Created index Apples_id_count_idx_btree on [id, count] of table Apples ▸ Created auto-increment constraint Bananas_id_seq on column id of table Bananas -▸ Created schedule for table Inspections_sched calling reducer perform_inspection +▸ Created schedule for table Inspections calling reducer perform_inspection ▸ ▸ Created anonymous view: my_view Parameters: • x: U32