From 2fb22678d5a5c3449d69109512de75950dfc8353 Mon Sep 17 00:00:00 2001 From: james gilles Date: Thu, 10 Oct 2024 12:22:51 -0400 Subject: [PATCH] [release/v0.12.1-beta]: Correct `Schema::check_compatible` to resolve types in a ModuleDef (#1813) --- crates/core/src/db/update.rs | 8 +++--- crates/schema/src/schema.rs | 27 ++++++++++--------- ...e_pseudomigration.py => auto_migration.py} | 19 ++++++++++--- 3 files changed, 34 insertions(+), 20 deletions(-) rename smoketests/tests/{add_table_pseudomigration.py => auto_migration.py} (89%) diff --git a/crates/core/src/db/update.rs b/crates/core/src/db/update.rs index 0705e6509e..9d88b99045 100644 --- a/crates/core/src/db/update.rs +++ b/crates/core/src/db/update.rs @@ -30,16 +30,16 @@ pub fn update_database( let existing_tables = stdb.get_all_tables_mut(tx)?; // TODO: consider using `ErrorStream` here. - let old_def = plan.old_def(); + let old_module_def = plan.old_def(); for table in existing_tables .iter() .filter(|table| table.table_type != StTableType::System) { - let old_def = old_def + let old_def = old_module_def .table(&table.table_name[..]) - .ok_or_else(|| anyhow::anyhow!("table {} not found in old_def", table.table_name))?; + .ok_or_else(|| anyhow::anyhow!("table {} not found in old_module_def", table.table_name))?; - table.check_compatible(old_def)?; + table.check_compatible(old_module_def, old_def)?; } match plan { diff --git a/crates/schema/src/schema.rs b/crates/schema/src/schema.rs index 46100b8192..25a1a2ab55 100644 --- a/crates/schema/src/schema.rs +++ b/crates/schema/src/schema.rs @@ -46,7 +46,7 @@ pub trait Schema: Sized { fn from_module_def(module_def: &ModuleDef, def: &Self::Def, parent_id: Self::ParentId, id: Self::Id) -> Self; /// Check that a schema entity is compatible with a definition. - fn check_compatible(&self, def: &Self::Def) -> Result<(), anyhow::Error>; + fn check_compatible(&self, module_def: &ModuleDef, def: &Self::Def) -> Result<(), anyhow::Error>; } /// A data structure representing the schema of a database table. @@ -602,7 +602,7 @@ impl Schema for TableSchema { ) } - fn check_compatible(&self, def: &Self::Def) -> Result<(), anyhow::Error> { + fn check_compatible(&self, module_def: &ModuleDef, def: &Self::Def) -> Result<(), anyhow::Error> { ensure_eq!(&self.table_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(); @@ -615,7 +615,7 @@ impl Schema for TableSchema { .columns .get(col.col_pos.0 as usize) .ok_or_else(|| anyhow::anyhow!("Column {} not found in definition", col.col_pos.0))?; - col.check_compatible(col_def)?; + col.check_compatible(module_def, col_def)?; } ensure_eq!(self.columns.len(), def.columns.len(), "Column count mismatch"); @@ -624,7 +624,7 @@ impl Schema for TableSchema { .indexes .get(&index.index_name[..]) .ok_or_else(|| anyhow::anyhow!("Index {} not found in definition", index.index_id.0))?; - index.check_compatible(index_def)?; + index.check_compatible(module_def, index_def)?; } ensure_eq!(self.indexes.len(), def.indexes.len(), "Index count mismatch"); @@ -633,7 +633,7 @@ impl Schema for TableSchema { .constraints .get(&constraint.constraint_name[..]) .ok_or_else(|| anyhow::anyhow!("Constraint {} not found in definition", constraint.constraint_id.0))?; - constraint.check_compatible(constraint_def)?; + constraint.check_compatible(module_def, constraint_def)?; } ensure_eq!( self.constraints.len(), @@ -646,7 +646,7 @@ impl Schema for TableSchema { .sequences .get(&sequence.sequence_name[..]) .ok_or_else(|| anyhow::anyhow!("Sequence {} not found in definition", sequence.sequence_id.0))?; - sequence.check_compatible(sequence_def)?; + sequence.check_compatible(module_def, sequence_def)?; } ensure_eq!(self.sequences.len(), def.sequences.len(), "Sequence count mismatch"); @@ -655,7 +655,7 @@ impl Schema for TableSchema { .schedule .as_ref() .ok_or_else(|| anyhow::anyhow!("Schedule not found in definition"))?; - schedule.check_compatible(schedule_def)?; + schedule.check_compatible(module_def, schedule_def)?; } ensure_eq!( self.schedule.is_some(), @@ -754,9 +754,10 @@ impl Schema for ColumnSchema { } } - fn check_compatible(&self, def: &Self::Def) -> Result<(), anyhow::Error> { + fn check_compatible(&self, module_def: &ModuleDef, def: &Self::Def) -> Result<(), anyhow::Error> { ensure_eq!(&self.col_name[..], &def.name[..], "Column name mismatch"); - ensure_eq!(self.col_type, def.ty, "Column type mismatch"); + let resolved_def_ty = WithTypespace::new(module_def.typespace(), &def.ty).resolve_refs()?; + ensure_eq!(self.col_type, resolved_def_ty, "Column type mismatch"); ensure_eq!(self.col_pos, def.col_id, "Columnh ID mismatch"); Ok(()) } @@ -843,7 +844,7 @@ impl Schema for SequenceSchema { } } - fn check_compatible(&self, def: &Self::Def) -> Result<(), anyhow::Error> { + fn check_compatible(&self, _module_def: &ModuleDef, def: &Self::Def) -> Result<(), anyhow::Error> { ensure_eq!(&self.sequence_name[..], &def.name[..], "Sequence name mismatch"); ensure_eq!(self.col_pos, def.column, "Sequence column mismatch"); ensure_eq!(self.increment, def.increment, "Sequence increment mismatch"); @@ -895,7 +896,7 @@ impl Schema for ScheduleSchema { } } - fn check_compatible(&self, def: &Self::Def) -> Result<(), anyhow::Error> { + fn check_compatible(&self, _module_def: &ModuleDef, def: &Self::Def) -> Result<(), anyhow::Error> { ensure_eq!(&self.schedule_name[..], &def.name[..], "Schedule name mismatch"); ensure_eq!( &self.reducer_name[..], @@ -939,7 +940,7 @@ impl Schema for IndexSchema { } } - fn check_compatible(&self, def: &Self::Def) -> Result<(), anyhow::Error> { + fn check_compatible(&self, _module_def: &ModuleDef, def: &Self::Def) -> Result<(), anyhow::Error> { ensure_eq!(&self.index_name[..], &def.name[..], "Index name mismatch"); ensure_eq!(&self.index_algorithm, &def.algorithm, "Index algorithm mismatch"); Ok(()) @@ -1002,7 +1003,7 @@ impl Schema for ConstraintSchema { } } - fn check_compatible(&self, def: &Self::Def) -> Result<(), anyhow::Error> { + fn check_compatible(&self, _module_def: &ModuleDef, def: &Self::Def) -> Result<(), anyhow::Error> { ensure_eq!(&self.constraint_name[..], &def.name[..], "Constraint name mismatch"); ensure_eq!(&self.data, &def.data, "Constraint data mismatch"); Ok(()) diff --git a/smoketests/tests/add_table_pseudomigration.py b/smoketests/tests/auto_migration.py similarity index 89% rename from smoketests/tests/add_table_pseudomigration.py rename to smoketests/tests/auto_migration.py index 10dba18df9..90c09e5b00 100644 --- a/smoketests/tests/add_table_pseudomigration.py +++ b/smoketests/tests/auto_migration.py @@ -3,9 +3,9 @@ import logging -class AddTablePseudomigration(Smoketest): +class AddTableAutoMigration(Smoketest): MODULE_CODE = """ -use spacetimedb::{println, ReducerContext, Table}; +use spacetimedb::{println, ReducerContext, Table, SpacetimeType}; #[spacetimedb::table(name = person)] pub struct Person { @@ -23,6 +23,19 @@ class AddTablePseudomigration(Smoketest): println!("{}: {}", prefix, person.name); } } + +#[spacetimedb::table(name = point_mass)] +pub struct PointMass { + mass: f64, + /// This used to cause an error when check_compatible did not resolve types in a `ModuleDef`. + position: Vector2, +} + +#[derive(SpacetimeType, Clone, Copy)] +pub struct Vector2 { + x: f64, + y: f64, +} """ MODULE_CODE_UPDATED = ( @@ -47,7 +60,7 @@ class AddTablePseudomigration(Smoketest): """ ) - def test_add_table_pseudomigration(self): + def test_add_table_auto_migration(self): """This tests uploading a module with a schema change that should not require clearing the database.""" logging.info("Initial publish complete")