Skip to content

Commit 075fa8f

Browse files
Shubham8287Shubham8287
committed
Address comments
1 parent e046c41 commit 075fa8f

4 files changed

Lines changed: 84 additions & 29 deletions

File tree

‎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/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 & 21 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,45 @@ 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) => {
377+
let field_value: Result<AlgebraicValue> =
378+
ty.deserialize(Deserializer::new(&mut reader)).or_else(|decode_error| {
388379
return Err(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+
387+
// Ensure there's only one default value.
396388
if col.default_value.is_some() {
397389
return Err(ValidationError::MultipleColumnDefaultValues {
398390
table: cdv.table.clone(),
399391
col_id: cdv.col_id,
400392
}
401393
.into());
402394
}
403-
col.default_value = field_value.clone();
395+
col.default_value = field_value.ok();
404396

405397
Ok(())
406398
}
@@ -953,6 +945,20 @@ fn check_scheduled_reducers_exist(
953945
.collect_all_errors()
954946
}
955947

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

0 commit comments

Comments
 (0)