From 9b7d65111271824345eecbff87c3763571ab94bd Mon Sep 17 00:00:00 2001 From: Ludv1g Date: Tue, 24 Feb 2026 02:59:22 +0100 Subject: [PATCH] feat: Allow adding unique constraints to existing tables with data validation precheck Instead of hard-rejecting #[unique] or #[primary_key] on existing columns, scan for duplicates first. If data is clean, allow the constraint. If duplicates exist, return a detailed error showing which values clash. Co-Authored-By: Claude Opus 4.6 --- crates/core/src/db/relational_db.rs | 11 ++- crates/core/src/db/update.rs | 68 ++++++++++++++++++- .../src/locking_tx_datastore/datastore.rs | 10 ++- .../src/locking_tx_datastore/mut_tx.rs | 2 +- crates/datastore/src/traits.rs | 7 +- crates/schema/src/auto_migrate.rs | 23 ++++--- crates/schema/src/auto_migrate/formatter.rs | 4 ++ 7 files changed, 109 insertions(+), 16 deletions(-) diff --git a/crates/core/src/db/relational_db.rs b/crates/core/src/db/relational_db.rs index 75fb75cbce7..4dca03a6cc6 100644 --- a/crates/core/src/db/relational_db.rs +++ b/crates/core/src/db/relational_db.rs @@ -50,7 +50,7 @@ use spacetimedb_sats::{AlgebraicType, AlgebraicValue, ProductType, ProductValue} use spacetimedb_schema::def::{ModuleDef, TableDef, ViewDef}; use spacetimedb_schema::reducer_name::ReducerName; use spacetimedb_schema::schema::{ - ColumnSchema, IndexSchema, RowLevelSecuritySchema, Schema, SequenceSchema, TableSchema, + ColumnSchema, ConstraintSchema, IndexSchema, RowLevelSecuritySchema, Schema, SequenceSchema, TableSchema, }; use spacetimedb_schema::table_name::TableName; use spacetimedb_snapshot::{ReconstructedSnapshot, SnapshotError, SnapshotRepository}; @@ -1483,6 +1483,15 @@ impl RelationalDB { Ok(self.inner.drop_sequence_mut_tx(tx, seq_id)?) } + /// Creates a new constraint in the database instance. + pub fn create_constraint( + &self, + tx: &mut MutTx, + constraint: ConstraintSchema, + ) -> Result { + Ok(self.inner.create_constraint_mut_tx(tx, constraint)?) + } + ///Removes the [Constraints] from database instance pub fn drop_constraint(&self, tx: &mut MutTx, constraint_id: ConstraintId) -> Result<(), DBError> { Ok(self.inner.drop_constraint_mut_tx(tx, constraint_id)?) diff --git a/crates/core/src/db/update.rs b/crates/core/src/db/update.rs index 896a1325e83..33cb9373e0f 100644 --- a/crates/core/src/db/update.rs +++ b/crates/core/src/db/update.rs @@ -5,7 +5,8 @@ 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::{ColList, ColSet, TableId}; +use spacetimedb_schema::schema::ConstraintSchema; 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}; @@ -133,6 +134,53 @@ fn auto_migrate_database( anyhow::bail!("Precheck failed: added sequence {sequence_name} already has values in range",); } } + spacetimedb_schema::auto_migrate::AutoMigratePrecheck::CheckAddUniqueConstraintValid( + constraint_name, + ) => { + let table_def = plan.new.stored_in_table_def(constraint_name).unwrap(); + let constraint_def = &table_def.constraints[constraint_name]; + let cols = constraint_def.data.unique_columns().unwrap(); + let col_list: ColList = cols.clone().into(); + let table_id = stdb.table_id_from_name_mut(tx, &table_def.name)?.unwrap(); + + // Scan all rows, count occurrences of each projected value + let mut value_counts: std::collections::HashMap = + std::collections::HashMap::new(); + for row in stdb.iter_mut(tx, table_id)? { + let val = row.project(&col_list)?; + *value_counts.entry(val).or_insert(0) += 1; + } + + // Collect duplicates (count > 1) + let duplicates: Vec<_> = value_counts + .into_iter() + .filter(|(_, count)| *count > 1) + .collect(); + + if !duplicates.is_empty() { + let total_groups = duplicates.len(); + let examples: String = duplicates + .iter() + .take(10) + .map(|(val, count)| format!(" - {val:?} appears {count} times")) + .collect::>() + .join("\n"); + let col_names: Vec<_> = cols + .iter() + .filter_map(|col_id| table_def.get_column(col_id).map(|c| c.name.to_string())) + .collect(); + anyhow::bail!( + "Precheck failed: cannot add unique constraint '{}' on table '{}' column(s) [{}]:\n\ + {} duplicate group(s) found.\n{}{}", + constraint_name, + table_def.name, + col_names.join(", "), + total_groups, + examples, + if total_groups > 10 { "\n ... and more" } else { "" } + ); + } + } } } @@ -220,6 +268,24 @@ fn auto_migrate_database( ); stdb.drop_constraint(tx, constraint_schema.constraint_id)?; } + spacetimedb_schema::auto_migrate::AutoMigrateStep::AddConstraint(constraint_name) => { + let table_def = plan.new.stored_in_table_def(constraint_name).unwrap(); + let constraint_def = &table_def.constraints[constraint_name]; + let table_id = stdb.table_id_from_name_mut(tx, &table_def.name)?.unwrap(); + let constraint_schema = ConstraintSchema::from_module_def( + plan.new, + constraint_def, + table_id, + spacetimedb_primitives::ConstraintId::SENTINEL, + ); + log!( + logger, + "Adding constraint `{}` on table `{}`", + constraint_name, + table_def.name + ); + stdb.create_constraint(tx, constraint_schema)?; + } 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(); diff --git a/crates/datastore/src/locking_tx_datastore/datastore.rs b/crates/datastore/src/locking_tx_datastore/datastore.rs index 46100f32eb1..c5f1a5c6907 100644 --- a/crates/datastore/src/locking_tx_datastore/datastore.rs +++ b/crates/datastore/src/locking_tx_datastore/datastore.rs @@ -42,7 +42,7 @@ use spacetimedb_sats::{memory_usage::MemoryUsage, Deserialize}; use spacetimedb_schema::table_name::TableName; use spacetimedb_schema::{ reducer_name::ReducerName, - schema::{ColumnSchema, IndexSchema, SequenceSchema, TableSchema}, + schema::{ColumnSchema, ConstraintSchema, IndexSchema, SequenceSchema, TableSchema}, }; use spacetimedb_snapshot::{ReconstructedSnapshot, SnapshotRepository}; use spacetimedb_table::{ @@ -575,6 +575,14 @@ impl MutTxDatastore for Locking { tx.sequence_id_from_name(sequence_name) } + fn create_constraint_mut_tx( + &self, + tx: &mut Self::MutTx, + constraint: ConstraintSchema, + ) -> Result { + tx.create_constraint(constraint) + } + fn drop_constraint_mut_tx(&self, tx: &mut Self::MutTx, constraint_id: ConstraintId) -> Result<()> { tx.drop_constraint(constraint_id) } diff --git a/crates/datastore/src/locking_tx_datastore/mut_tx.rs b/crates/datastore/src/locking_tx_datastore/mut_tx.rs index 869205cf2ff..cd6afa4d722 100644 --- a/crates/datastore/src/locking_tx_datastore/mut_tx.rs +++ b/crates/datastore/src/locking_tx_datastore/mut_tx.rs @@ -1816,7 +1816,7 @@ impl MutTxId { /// Ensures: /// - The constraint metadata is inserted into the system tables (and other data structures reflecting them). /// - The returned ID is unique and is not `constraintId::SENTINEL`. - fn create_constraint(&mut self, mut constraint: ConstraintSchema) -> Result { + pub fn create_constraint(&mut self, mut constraint: ConstraintSchema) -> Result { if constraint.table_id == TableId::SENTINEL { return Err(anyhow::anyhow!("`table_id` must not be `TableId::SENTINEL` in `{constraint:#?}`").into()); } diff --git a/crates/datastore/src/traits.rs b/crates/datastore/src/traits.rs index 9b98c11addf..c05b2d67443 100644 --- a/crates/datastore/src/traits.rs +++ b/crates/datastore/src/traits.rs @@ -15,7 +15,7 @@ use spacetimedb_primitives::*; use spacetimedb_sats::hash::Hash; use spacetimedb_sats::{AlgebraicValue, ProductType, ProductValue}; use spacetimedb_schema::reducer_name::ReducerName; -use spacetimedb_schema::schema::{IndexSchema, SequenceSchema, TableSchema}; +use spacetimedb_schema::schema::{ConstraintSchema, IndexSchema, SequenceSchema, TableSchema}; use spacetimedb_schema::table_name::TableName; use spacetimedb_table::static_assert_size; use spacetimedb_table::table::RowRef; @@ -638,6 +638,11 @@ pub trait MutTxDatastore: TxDatastore + MutTx { fn sequence_id_from_name_mut_tx(&self, tx: &Self::MutTx, sequence_name: &str) -> super::Result>; // Constraints + fn create_constraint_mut_tx( + &self, + tx: &mut Self::MutTx, + constraint: ConstraintSchema, + ) -> super::Result; fn drop_constraint_mut_tx(&self, tx: &mut Self::MutTx, constraint_id: ConstraintId) -> super::Result<()>; fn constraint_id_from_name(&self, tx: &Self::MutTx, constraint_name: &str) -> super::Result>; diff --git a/crates/schema/src/auto_migrate.rs b/crates/schema/src/auto_migrate.rs index ab4dfc14fb4..0b47bb4f559 100644 --- a/crates/schema/src/auto_migrate.rs +++ b/crates/schema/src/auto_migrate.rs @@ -220,6 +220,9 @@ pub enum AutoMigratePrecheck<'def> { /// 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>), + /// Perform a check that adding a unique constraint is valid (no duplicate values exist + /// in the relevant columns). + CheckAddUniqueConstraintValid(::Key<'def>), } /// A step in an automatic migration. @@ -277,6 +280,8 @@ pub enum AutoMigrateStep<'def> { AddTable(::Key<'def>), /// Add an index. AddIndex(::Key<'def>), + /// Add a constraint to an existing table (with data validation precheck). + AddConstraint(::Key<'def>), /// Add a sequence. AddSequence(::Key<'def>), /// Add a schedule annotation to a table. @@ -1002,11 +1007,11 @@ fn auto_migrate_constraints(plan: &mut AutoMigratePlan, new_tables: &HashSet<&Id // 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()) + // existing table — validate data for duplicates, then add constraint + plan.prechecks + .push(AutoMigratePrecheck::CheckAddUniqueConstraintValid(new.key())); + plan.steps.push(AutoMigrateStep::AddConstraint(new.key())); + Ok(()) } } Diff::Remove { old } => { @@ -1505,8 +1510,6 @@ mod tests { let apples = expect_identifier("Apples"); let bananas = expect_identifier("Bananas"); - let apples_name_unique_constraint = "Apples_name_key"; - let weight = expect_identifier("weight"); let count = expect_identifier("count"); let name = expect_identifier("name"); @@ -1701,10 +1704,8 @@ mod tests { && type1.0 == prod1_ty && type2.0 == new_prod1_ty ); - expect_error_matching!( - result, - AutoMigrateError::AddUniqueConstraint { constraint } => &constraint[..] == apples_name_unique_constraint - ); + // Note: AddUniqueConstraint is no longer an error - adding unique constraints + // to existing tables is now allowed with a data validation precheck. expect_error_matching!( result, diff --git a/crates/schema/src/auto_migrate/formatter.rs b/crates/schema/src/auto_migrate/formatter.rs index a3edad35b97..d46175814dd 100644 --- a/crates/schema/src/auto_migrate/formatter.rs +++ b/crates/schema/src/auto_migrate/formatter.rs @@ -58,6 +58,10 @@ fn format_step( let constraint_info = extract_constraint_info(*constraint, plan.old)?; f.format_constraint(&constraint_info, Action::Removed) } + AutoMigrateStep::AddConstraint(constraint) => { + let constraint_info = extract_constraint_info(*constraint, plan.new)?; + f.format_constraint(&constraint_info, Action::Created) + } AutoMigrateStep::AddSequence(sequence) => { let sequence_info = extract_sequence_info(*sequence, plan.new)?; f.format_sequence(&sequence_info, Action::Created)