Skip to content

Commit 4bd8ce7

Browse files
Shubham8287Shubham8287
committed
Address comments
1 parent e046c41 commit 4bd8ce7

File tree

5 files changed

+95
-35
lines changed

5 files changed

+95
-35
lines changed

‎crates/core/src/db/update.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ fn auto_migrate_database(
252252
log!(logger, "Removing-row level security `{sql_rls}`");
253253
stdb.drop_row_level_security(tx, sql_rls.clone())?;
254254
}
255-
_ => unimplemented!(),
255+
_ => anyhow::bail!("migration step not implemented: {step:?}"),
256256
}
257257
}
258258

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

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -366,19 +366,18 @@ pub enum RawMiscModuleExportV9 {
366366
ColumnDefaultValue(RawColumnDefaultValueV9),
367367
}
368368

369-
/// Marks a field as having a particularly default
369+
/// Marks a field as having a particular default.
370370
#[derive(Debug, Clone, SpacetimeType)]
371371
#[sats(crate = crate)]
372372
#[cfg_attr(feature = "test", derive(PartialEq, Eq, PartialOrd, Ord))]
373373
pub struct RawColumnDefaultValueV9 {
374-
/// Must point to a `ProductType` or an error will be thrown during validation.
375374
pub table: RawIdentifier,
376375

377376
/// Must be the index of a valid column within `ty`.
378377
pub col_id: ColId,
379378

380-
/// A BSATN-encoded AlgebraicValue valid at `ty`.
381-
/// (We can't use AlgebraicValue directly because it isn't serializable!)
379+
/// A BSATN-encoded [`AlgebraicValue`] valid at `ty`.
380+
/// (We can't use `AlgebraicValue` directly because it isn't serializable!)
382381
pub value: Box<[u8]>,
383382
}
384383

@@ -813,10 +812,9 @@ impl RawTableDefBuilder<'_> {
813812
self
814813
}
815814

816-
/// Adds a default value for the field.
817-
/// Note: this sets the value for all other tables defined on the same
815+
/// Adds a default value for the `column`.
818816
pub fn with_default_column_value(self, column: impl Into<ColId>, value: AlgebraicValue) -> Self {
819-
// Added to misc_exports for backwards-compatibility reasons
817+
// Added to `misc_exports` for backwards-compatibility reasons.
820818
self.module_def
821819
.misc_exports
822820
.push(RawMiscModuleExportV9::ColumnDefaultValue(RawColumnDefaultValueV9 {

‎crates/schema/src/auto_migrate.rs

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -113,10 +113,12 @@ pub enum AutoMigrateStep<'def> {
113113
///
114114
/// This is a destructive operation that requires first running a `DisconnectAllUsers`.
115115
///
116-
/// The added columns are guaranteed to be contiguous and at the end of the table. They are also
117-
/// guaranteed to have default values set.
116+
/// The added columns are guaranteed to be contiguous
117+
/// and at the end of the table.
118+
/// They are also guaranteed to have default values set.
118119
///
119-
/// This suppresses any `ChangeColumns` steps for the same table.
120+
/// When this step is present,
121+
/// no `ChangeColumns` steps will be, for the same table.
120122
AddColumns(<TableDef as ModuleDefLookup>::Key<'def>),
121123

122124
/// 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
400402
match col_diff {
401403
Diff::Add { new } => {
402404
if new.default_value.is_some() {
403-
// row_type_changed, columns_added
405+
// `row_type_changed`, `columns_added`
404406
Ok(ProductMonoid(Any(false), Any(true)))
405407
} else {
406408
Err(AutoMigrateError::AddColumn {
@@ -451,6 +453,8 @@ fn auto_migrate_table<'def>(plan: &mut AutoMigratePlan<'def>, old: &'def TableDe
451453

452454
let ((), ProductMonoid(Any(row_type_changed), Any(columns_added))) = (type_ok, columns_ok).combine_errors()?;
453455

456+
// If we're adding a column, we'll rewrite the whole table.
457+
// That makes any `ChangeColumns` moot, so we can skip it.
454458
if columns_added {
455459
if !plan
456460
.steps
@@ -875,7 +879,7 @@ mod tests {
875879
)
876880
// add column sequence
877881
.with_column_sequence(0)
878-
.with_default_column_value(3, AlgebraicValue::U32(5)) // we need a new
882+
.with_default_column_value(3, AlgebraicValue::U32(5))
879883
// change access
880884
.with_access(TableAccess::Private)
881885
.finish();
@@ -1024,6 +1028,8 @@ mod tests {
10241028

10251029
assert!(steps.contains(&AutoMigrateStep::DisconnectAllUsers), "{steps:?}");
10261030
assert!(steps.contains(&AutoMigrateStep::AddColumns(&bananas)), "{steps:?}");
1031+
// Column is changed but it will not reflect in steps due to `AutoMigrateStep::AddColumns`
1032+
assert!(!steps.contains(&AutoMigrateStep::ChangeColumns(&bananas)), "{steps:?}");
10271033
}
10281034

10291035
#[test]

‎crates/schema/src/def.rs

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1059,8 +1059,12 @@ where
10591059

10601060
#[cfg(test)]
10611061
mod tests {
1062+
use crate::{def::validate::tests::expect_identifier, error::ValidationError};
1063+
10621064
use super::*;
10631065
use proptest::prelude::*;
1066+
use spacetimedb_data_structures::expect_error_matching;
1067+
use spacetimedb_lib::db::raw_def::v9::RawModuleDefV9Builder;
10641068

10651069
proptest! {
10661070
#[test]
@@ -1076,4 +1080,51 @@ mod tests {
10761080
prop_assert_eq!(raw, raw2);
10771081
}
10781082
}
1083+
1084+
#[test]
1085+
fn validate_new_column_with_multiple_values() {
1086+
let mut old_builder = RawModuleDefV9Builder::new();
1087+
old_builder
1088+
.build_table_with_new_type(
1089+
"Apples",
1090+
ProductType::from([("id", AlgebraicType::U64), ("count", AlgebraicType::U16)]),
1091+
true,
1092+
)
1093+
.with_default_column_value(1, AlgebraicValue::U16(12))
1094+
.with_default_column_value(1, AlgebraicValue::U16(10))
1095+
.finish();
1096+
1097+
let result: Result<ModuleDef, ValidationErrors> = old_builder.finish().try_into();
1098+
let apples = expect_identifier("Apples");
1099+
1100+
expect_error_matching!(
1101+
result,
1102+
ValidationError::MultipleColumnDefaultValues {
1103+
table,
1104+
..
1105+
} => *table == apples.clone().into()
1106+
);
1107+
}
1108+
1109+
#[test]
1110+
fn validate_new_column_with_malformed_value() {
1111+
let mut old_builder = RawModuleDefV9Builder::new();
1112+
old_builder
1113+
.build_table_with_new_type(
1114+
"Apples",
1115+
ProductType::from([("id", AlgebraicType::U64), ("count", AlgebraicType::U16)]),
1116+
true,
1117+
)
1118+
.with_default_column_value(1, AlgebraicValue::Bool(false))
1119+
.with_default_column_value(1, AlgebraicValue::U16(10))
1120+
.finish();
1121+
1122+
let result: Result<ModuleDef, ValidationErrors> = old_builder.finish().try_into();
1123+
let apples = expect_identifier("Apples");
1124+
1125+
expect_error_matching!(
1126+
result,
1127+
ValidationError::ColumnDefaultValueMalformed { table, col_id, .. } => *table == apples.clone().into() && *col_id == ColId(1)
1128+
);
1129+
}
10791130
}

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

Lines changed: 27 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -80,16 +80,7 @@ pub fn validate(def: RawModuleDefV9) -> Result<ModuleDef> {
8080
.combine_errors()
8181
.and_then(|(mut tables, types, reducers)| {
8282
let sched_exists = check_scheduled_reducers_exist(&tables, &reducers);
83-
let default_values_work = misc_exports
84-
.into_iter()
85-
.map(|export| match export {
86-
RawMiscModuleExportV9::ColumnDefaultValue(fdv) => {
87-
validator.validate_column_default_value(&mut tables, fdv)
88-
}
89-
_ => unimplemented!("unknown misc export"),
90-
})
91-
.collect_all_errors::<()>();
92-
83+
let default_values_work = default_values_work(misc_exports, &mut validator, &mut tables);
9384
(sched_exists, default_values_work).combine_errors()?;
9485

9586
Ok((tables, types, reducers))
@@ -363,44 +354,44 @@ impl ModuleValidator<'_> {
363354
) -> Result<()> {
364355
let table_name = identifier(cdv.table.clone())?;
365356

357+
// Extract the table. We cannot make progress otherwise.
366358
let table = tables
367359
.get_mut(&table_name)
368360
.ok_or_else(|| ValidationError::TableNotFound {
369361
table: cdv.table.clone(),
370362
})?;
371363

372-
if table.columns.len() <= cdv.col_id.idx() {
364+
// Get the column that a default is being added to.
365+
let Some(col) = table.columns.get_mut(cdv.col_id.idx()) else {
373366
return Err(ValidationError::ColumnNotFound {
374367
table: cdv.table.clone(),
375368
def: cdv.table.clone(),
376369
column: cdv.col_id,
377370
}
378371
.into());
379-
}
372+
};
380373

381-
let col = &mut table.columns[cdv.col_id.idx()];
382-
// First time we have a type for it, so decode it.
374+
// First time the type of the default value is known, so decode it.
383375
let mut reader = &cdv.value[..];
384376
let ty = WithTypespace::new(self.typespace, &col.ty);
385-
let field_value = match ty.deserialize(Deserializer::new(&mut reader)) {
386-
Ok(field_value) => Some(field_value),
387-
Err(decode_error) => {
388-
return Err(ValidationError::ColumnDefaultValueMalformed {
377+
let field_value: Result<AlgebraicValue> =
378+
ty.deserialize(Deserializer::new(&mut reader)).map_err(|decode_error| {
379+
ValidationError::ColumnDefaultValueMalformed {
389380
table: cdv.table.clone(),
390381
col_id: cdv.col_id,
391382
err: decode_error,
392383
}
393-
.into())
394-
}
395-
};
384+
.into()
385+
});
386+
// Ensure there's only one default value.
396387
if col.default_value.is_some() {
397388
return Err(ValidationError::MultipleColumnDefaultValues {
398389
table: cdv.table.clone(),
399390
col_id: cdv.col_id,
400391
}
401392
.into());
402393
}
403-
col.default_value = field_value.clone();
394+
col.default_value = field_value.ok();
404395

405396
Ok(())
406397
}
@@ -953,6 +944,20 @@ fn check_scheduled_reducers_exist(
953944
.collect_all_errors()
954945
}
955946

947+
fn default_values_work(
948+
misc_exports: Vec<RawMiscModuleExportV9>,
949+
validator: &mut ModuleValidator,
950+
tables: &mut IdentifierMap<TableDef>,
951+
) -> Result<()> {
952+
misc_exports
953+
.into_iter()
954+
.map(|export| match export {
955+
RawMiscModuleExportV9::ColumnDefaultValue(fdv) => validator.validate_column_default_value(tables, fdv),
956+
_ => unimplemented!("unknown misc export"),
957+
})
958+
.collect_all_errors::<()>()
959+
}
960+
956961
#[cfg(test)]
957962
mod tests {
958963
use crate::def::validate::tests::{

0 commit comments

Comments
 (0)