Address comments

Author: Shubham8287

crates/core/src/db/update.rs

@@ -252,7 +252,7 @@ fn auto_migrate_database(
                 log!(logger, "Removing-row level security `{sql_rls}`");
                 stdb.drop_row_level_security(tx, sql_rls.clone())?;
             }
-            _ => unimplemented!(),
+            _ => anyhow::bail!("migration step not implemented: {step:?}"),
         }
     }
 

crates/lib/src/db/raw_def/v9.rs

@@ -366,19 +366,18 @@ pub enum RawMiscModuleExportV9 {
     ColumnDefaultValue(RawColumnDefaultValueV9),
 }
 
-/// Marks a field as having a particularly default
+/// Marks a field as having a particular default.
 #[derive(Debug, Clone, SpacetimeType)]
 #[sats(crate = crate)]
 #[cfg_attr(feature = "test", derive(PartialEq, Eq, PartialOrd, Ord))]
 pub struct RawColumnDefaultValueV9 {
-    /// Must point to a `ProductType` or an error will be thrown during validation.
     pub table: RawIdentifier,
 
     /// Must be the index of a valid column within `ty`.
     pub col_id: ColId,
 
-    /// A BSATN-encoded AlgebraicValue valid at `ty`.
-    /// (We can't use AlgebraicValue directly because it isn't serializable!)
+    /// A BSATN-encoded [`AlgebraicValue`] valid at `ty`.
+    /// (We can't use `AlgebraicValue` directly because it isn't serializable!)
     pub value: Box<[u8]>,
 }
 
@@ -813,10 +812,9 @@ impl RawTableDefBuilder<'_> {
         self
     }
 
-    /// Adds a default value for the field.
-    /// Note: this sets the value for all other tables defined on the same
+    /// Adds a default value for the `column`.
     pub fn with_default_column_value(self, column: impl Into<ColId>, value: AlgebraicValue) -> Self {
-        // Added to misc_exports for backwards-compatibility reasons
+        // Added to `misc_exports` for backwards-compatibility reasons.
         self.module_def
             .misc_exports
             .push(RawMiscModuleExportV9::ColumnDefaultValue(RawColumnDefaultValueV9 {

crates/schema/src/auto_migrate.rs

@@ -113,10 +113,12 @@ pub enum AutoMigrateStep<'def> {
     ///
     /// 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. They are also
-    /// guaranteed to have default values set.
+    /// The added columns are guaranteed to be contiguous
+    /// and at the end of the table.
+    /// They are also guaranteed to have default values set.
     ///
-    /// This suppresses any `ChangeColumns` steps for the same table.
+    /// When this step is present,
+    /// no `ChangeColumns` steps will be, for the same table.
     AddColumns(<TableDef as ModuleDefLookup>::Key<'def>),
 
     /// Add a table, including all indexes, constraints, and sequences.
@@ -400,7 +402,7 @@ fn auto_migrate_table<'def>(plan: &mut AutoMigratePlan<'def>, old: &'def TableDe
         match col_diff {
             Diff::Add { new } => {
                 if new.default_value.is_some() {
-                    // row_type_changed, columns_added
+                    // `row_type_changed`, `columns_added`
                     Ok(ProductMonoid(Any(false), Any(true)))
                 } else {
                     Err(AutoMigrateError::AddColumn {
@@ -451,6 +453,8 @@ fn auto_migrate_table<'def>(plan: &mut AutoMigratePlan<'def>, old: &'def TableDe
 
     let ((), ProductMonoid(Any(row_type_changed), Any(columns_added))) = (type_ok, columns_ok).combine_errors()?;
 
+    // If we're adding a column, we'll rewrite the whole table.
+    // That makes any `ChangeColumns` moot, so we can skip it.
     if columns_added {
         if !plan
             .steps
@@ -875,7 +879,7 @@ mod tests {
             )
             // add column sequence
             .with_column_sequence(0)
-            .with_default_column_value(3, AlgebraicValue::U32(5)) // we need a new
+            .with_default_column_value(3, AlgebraicValue::U32(5))
             // change access
             .with_access(TableAccess::Private)
             .finish();
@@ -1024,6 +1028,8 @@ mod tests {
 
         assert!(steps.contains(&AutoMigrateStep::DisconnectAllUsers), "{steps:?}");
         assert!(steps.contains(&AutoMigrateStep::AddColumns(&bananas)), "{steps:?}");
+        // Column is changed but it will not reflect in steps due to `AutoMigrateStep::AddColumns`
+        assert!(!steps.contains(&AutoMigrateStep::ChangeColumns(&bananas)), "{steps:?}");
     }
 
     #[test]

crates/schema/src/def.rs

@@ -1059,8 +1059,12 @@ where
 
 #[cfg(test)]
 mod tests {
+    use crate::{def::validate::tests::expect_identifier, error::ValidationError};
+
     use super::*;
     use proptest::prelude::*;
+    use spacetimedb_data_structures::expect_error_matching;
+    use spacetimedb_lib::db::raw_def::v9::RawModuleDefV9Builder;
 
     proptest! {
         #[test]
@@ -1076,4 +1080,51 @@ mod tests {
             prop_assert_eq!(raw, raw2);
         }
     }
+
+    #[test]
+    fn validate_new_column_with_multiple_values() {
+        let mut old_builder = RawModuleDefV9Builder::new();
+        old_builder
+            .build_table_with_new_type(
+                "Apples",
+                ProductType::from([("id", AlgebraicType::U64), ("count", AlgebraicType::U16)]),
+                true,
+            )
+            .with_default_column_value(1, AlgebraicValue::U16(12))
+            .with_default_column_value(1, AlgebraicValue::U16(10))
+            .finish();
+
+        let result: Result<ModuleDef, ValidationErrors> = old_builder.finish().try_into();
+        let apples = expect_identifier("Apples");
+
+        expect_error_matching!(
+            result,
+            ValidationError::MultipleColumnDefaultValues {
+                table,
+                ..
+            } => *table == apples.clone().into()
+        );
+    }
+
+    #[test]
+    fn validate_new_column_with_malformed_value() {
+        let mut old_builder = RawModuleDefV9Builder::new();
+        old_builder
+            .build_table_with_new_type(
+                "Apples",
+                ProductType::from([("id", AlgebraicType::U64), ("count", AlgebraicType::U16)]),
+                true,
+            )
+            .with_default_column_value(1, AlgebraicValue::Bool(false))
+            .with_default_column_value(1, AlgebraicValue::U16(10))
+            .finish();
+
+        let result: Result<ModuleDef, ValidationErrors> = old_builder.finish().try_into();
+        let apples = expect_identifier("Apples");
+
+        expect_error_matching!(
+            result,
+            ValidationError::ColumnDefaultValueMalformed { table, col_id, .. } => *table == apples.clone().into() && *col_id == ColId(1)
+        );
+    }
 }

crates/schema/src/def/validate/v9.rs

@@ -80,16 +80,7 @@ pub fn validate(def: RawModuleDefV9) -> Result<ModuleDef> {
         .combine_errors()
         .and_then(|(mut tables, types, reducers)| {
             let sched_exists = check_scheduled_reducers_exist(&tables, &reducers);
-            let default_values_work = misc_exports
-                .into_iter()
-                .map(|export| match export {
-                    RawMiscModuleExportV9::ColumnDefaultValue(fdv) => {
-                        validator.validate_column_default_value(&mut tables, fdv)
-                    }
-                    _ => unimplemented!("unknown misc export"),
-                })
-                .collect_all_errors::<()>();
-
+            let default_values_work = default_values_work(misc_exports, &mut validator, &mut tables);
             (sched_exists, default_values_work).combine_errors()?;
 
             Ok((tables, types, reducers))
@@ -363,44 +354,44 @@ impl ModuleValidator<'_> {
     ) -> Result<()> {
         let table_name = identifier(cdv.table.clone())?;
 
+        // Extract the table. We cannot make progress otherwise.
         let table = tables
             .get_mut(&table_name)
             .ok_or_else(|| ValidationError::TableNotFound {
                 table: cdv.table.clone(),
             })?;
 
-        if table.columns.len() <= cdv.col_id.idx() {
+        // Get the column that a default is being added to.
+        let Some(col) = table.columns.get_mut(cdv.col_id.idx()) else {
             return Err(ValidationError::ColumnNotFound {
                 table: cdv.table.clone(),
                 def: cdv.table.clone(),
                 column: cdv.col_id,
             }
             .into());
-        }
+        };
 
-        let col = &mut table.columns[cdv.col_id.idx()];
-        // First time we have a type for it, so decode it.
+        // First time the type of the default value is known, so decode it.
         let mut reader = &cdv.value[..];
         let ty = WithTypespace::new(self.typespace, &col.ty);
-        let field_value = match ty.deserialize(Deserializer::new(&mut reader)) {
-            Ok(field_value) => Some(field_value),
-            Err(decode_error) => {
-                return Err(ValidationError::ColumnDefaultValueMalformed {
+        let field_value: Result<AlgebraicValue> =
+            ty.deserialize(Deserializer::new(&mut reader)).map_err(|decode_error| {
+                ValidationError::ColumnDefaultValueMalformed {
                     table: cdv.table.clone(),
                     col_id: cdv.col_id,
                     err: decode_error,
                 }
-                .into())
-            }
-        };
+                .into()
+            });
+        // Ensure there's only one default value.
         if col.default_value.is_some() {
             return Err(ValidationError::MultipleColumnDefaultValues {
                 table: cdv.table.clone(),
                 col_id: cdv.col_id,
             }
             .into());
         }
-        col.default_value = field_value.clone();
+        col.default_value = field_value.ok();
 
         Ok(())
     }
@@ -953,6 +944,20 @@ fn check_scheduled_reducers_exist(
         .collect_all_errors()
 }
 
+fn default_values_work(
+    misc_exports: Vec<RawMiscModuleExportV9>,
+    validator: &mut ModuleValidator,
+    tables: &mut IdentifierMap<TableDef>,
+) -> Result<()> {
+    misc_exports
+        .into_iter()
+        .map(|export| match export {
+            RawMiscModuleExportV9::ColumnDefaultValue(fdv) => validator.validate_column_default_value(tables, fdv),
+            _ => unimplemented!("unknown misc export"),
+        })
+        .collect_all_errors::<()>()
+}
+
 #[cfg(test)]
 mod tests {
     use crate::def::validate::tests::{