Add backwards-compatible validation for RawModuleDefV8. (#1606)

Author: kazimuth

Cargo.lock

@@ -85,7 +85,7 @@
 //! and `CollectAllErrors::collect_all_errors` for collecting errors from iterators.
 
 use crate::map::HashSet;
-use std::hash::Hash;
+use std::{fmt, hash::Hash};
 
 /// A non-empty stream of errors.
 ///
@@ -115,6 +115,41 @@ use std::hash::Hash;
 pub struct ErrorStream<E>(smallvec::SmallVec<[E; 1]>);
 
 impl<E> ErrorStream<E> {
+    /// Build an error stream from a non-empty collection.
+    /// If the collection is empty, panic.
+    pub fn expect_nonempty<I: IntoIterator<Item = E>>(errors: I) -> Self {
+        let mut errors = errors.into_iter();
+        let first = errors.next().expect("expected at least one error");
+        let mut stream = Self::from(first);
+        stream.extend(errors);
+        stream
+    }
+
+    /// Add some extra errors to a result.
+    ///
+    /// If there are no errors, the result is not modified.
+    /// If there are errors, and the result is `Err`, the errors are added to the stream.
+    /// If there are errors, and the result is `Ok`, the `Ok` value is discarded, and the errors are returned in a stream.
+    pub fn add_extra_errors<T>(
+        result: Result<T, ErrorStream<E>>,
+        extra_errors: impl IntoIterator<Item = E>,
+    ) -> Result<T, ErrorStream<E>> {
+        match result {
+            Ok(value) => {
+                let errors: SmallVec<[E; 1]> = extra_errors.into_iter().collect();
+                if errors.is_empty() {
+                    Ok(value)
+                } else {
+                    Err(ErrorStream(errors))
+                }
+            }
+            Err(mut errors) => {
+                errors.extend(extra_errors);
+                Err(errors)
+            }
+        }
+    }
+
     /// Returns an iterator over the errors in the stream.
     pub fn iter(&self) -> impl Iterator<Item = &E> {
         self.0.iter()
@@ -154,6 +189,15 @@ impl<E> ErrorStream<E> {
         }
     }
 }
+impl<E: fmt::Display> fmt::Display for ErrorStream<E> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        writeln!(f, "Errors occurred:")?;
+        for error in self.iter() {
+            writeln!(f, "{}\n", error)?;
+        }
+        Ok(())
+    }
+}
 
 impl<E: Ord + Eq> ErrorStream<E> {
     /// Sort and deduplicate the errors in the error stream.
@@ -454,6 +498,7 @@ macro_rules! expect_error_matching (
 );
 // Make available in this module as well as crate root.
 pub use expect_error_matching;
+use smallvec::SmallVec;
 
 #[cfg(test)]
 mod tests {

crates/data-structures/src/error_stream.rs

@@ -10,9 +10,10 @@ use spacetimedb_data_structures::map::HashSet;
 use spacetimedb_primitives::*;
 use spacetimedb_sats::{de, ser};
 
-// TODO(1.0): move this definition into this file,
+// TODO(1.0): move these definitions into this file,
 // along with the other structs contained in it,
 // which are currently in the crate root.
+pub use crate::ModuleDefBuilder as RawModuleDefV8Builder;
 pub use crate::RawModuleDefV8;
 
 /// The amount sequences allocate each time they over-run their allocation.
@@ -343,6 +344,11 @@ impl RawTableDefV8 {
         }
     }
 
+    #[cfg(feature = "test")]
+    pub fn new_for_tests(table_name: impl Into<Box<str>>, columns: ProductType) -> Self {
+        Self::new(table_name.into(), RawColumnDefV8::from_product_type(columns))
+    }
+
     /// Set the type of the table and return a new `TableDef` instance with the updated type.
     pub fn with_type(self, table_type: StTableType) -> Self {
         let mut x = self;

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

@@ -493,6 +493,21 @@ impl RawModuleDefV9Builder {
     }
 }
 
+/// Convert a string from a sats type-name annotation like `#[sats(name = "namespace.name")]` to a `RawScopedTypeNameV9`.
+/// We split the input on the strings `"::"` and `"."` to split up module paths.
+///
+/// TODO(1.0): build namespacing directly into the bindings macros so that we don't need to do this.
+pub fn sats_name_to_scoped_name(sats_name: &str) -> RawScopedTypeNameV9 {
+    // We can't use `&[char]: Pattern` for `split` here because "::" is not a char :/
+    let mut scope: Vec<RawIdentifier> = sats_name.split("::").flat_map(|s| s.split('.')).map_into().collect();
+    // Unwrapping to "" will result in a validation error down the line, which is exactly what we want.
+    let name = scope.pop().unwrap_or_default();
+    RawScopedTypeNameV9 {
+        scope: scope.into(),
+        name,
+    }
+}
+
 impl TypespaceBuilder for RawModuleDefV9Builder {
     fn add(
         &mut self,
@@ -509,19 +524,11 @@ impl TypespaceBuilder for RawModuleDefV9Builder {
                 v.insert(slot_ref);
 
                 // Alias provided? Relate `name -> slot_ref`.
-                if let Some(name) = name {
-                    // Right now, we just split the name on patterns likely to split up module paths.
-                    // TODO(1.0): build namespacing directly into the bindings macros so that we don't need to do this.
-                    // Note that we can't use `&[char]: Pattern` for `split` here because "::" is not a char :/
-                    let mut scope: Vec<RawIdentifier> =
-                        name.split("::").flat_map(|s| s.split('.')).map_into().collect();
-                    let name = scope.pop().expect("empty name forbidden");
+                if let Some(sats_name) = name {
+                    let name = sats_name_to_scoped_name(sats_name);
 
                     self.module.types.push(RawTypeDefV9 {
-                        name: RawScopedTypeNameV9 {
-                            name,
-                            scope: scope.into(),
-                        },
+                        name,
                         ty: slot_ref,
                         // TODO(1.0): we need to update the `TypespaceBuilder` trait to include
                         // a `custom_ordering` parameter.
@@ -622,8 +629,9 @@ impl<'a> RawTableDefBuilder<'a> {
         self
     }
 
-    /// Build the table and add it to the module.
-    pub fn finish(self) {
+    /// Build the table and add it to the module, returning the `product_type_ref` of the table.
+    pub fn finish(self) -> AlgebraicTypeRef {
+        self.table.product_type_ref
         // self is now dropped.
     }
 

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

@@ -222,6 +222,38 @@ impl ModuleDefBuilder {
         TypespaceBuilder::add_type::<T>(self)
     }
 
+    /// Add a type that may not correspond to a Rust type.
+    /// Used only in tests.
+    #[cfg(feature = "test")]
+    pub fn add_type_for_tests(&mut self, name: &str, ty: AlgebraicType) -> spacetimedb_sats::AlgebraicTypeRef {
+        let slot_ref = self.module.typespace.add(ty);
+        self.module.misc_exports.push(MiscModuleExport::TypeAlias(TypeAlias {
+            name: name.to_owned(),
+            ty: slot_ref,
+        }));
+        slot_ref
+    }
+
+    /// Add a table that may not correspond to a Rust type.
+    /// Wraps it in a `TableDesc` and generates a corresponding `ProductType` in the typespace.
+    /// Used only in tests.
+    /// Returns the `AlgebraicTypeRef` of the generated `ProductType`.
+    #[cfg(feature = "test")]
+    pub fn add_table_for_tests(&mut self, schema: RawTableDefV8) -> spacetimedb_sats::AlgebraicTypeRef {
+        let ty: ProductType = schema
+            .columns
+            .iter()
+            .map(|c| ProductTypeElement {
+                name: Some(c.col_name.clone()),
+                algebraic_type: c.col_type.clone(),
+            })
+            .collect();
+        // do NOT add a `TypeAlias`: in v8, the `RawTableDef` itself serves as a `TypeAlias`.
+        let data = self.module.typespace.add(ty.into());
+        self.add_table(TableDesc { schema, data });
+        data
+    }
+
     pub fn add_table(&mut self, table: TableDesc) {
         self.module.tables.push(table)
     }
@@ -230,6 +262,14 @@ impl ModuleDefBuilder {
         self.module.reducers.push(reducer)
     }
 
+    #[cfg(feature = "test")]
+    pub fn add_reducer_for_tests(&mut self, name: impl Into<Box<str>>, args: ProductType) {
+        self.add_reducer(ReducerDef {
+            name: name.into(),
+            args: args.elements.to_vec(),
+        });
+    }
+
     pub fn add_misc_export(&mut self, misc_export: MiscModuleExport) {
         self.module.misc_exports.push(misc_export)
     }

crates/lib/src/lib.rs

@@ -21,4 +21,8 @@ smallvec.workspace = true
 
 [dev-dependencies]
 spacetimedb-lib = { workspace = true, features = ["test"] }
+# these are circular dependencies, but only in tests, so it's fine
+spacetimedb-testing = { path = "../testing" }
+spacetimedb-cli.workspace = true
+
 proptest.workspace = true

crates/schema/Cargo.toml

@@ -90,6 +90,17 @@ pub struct ModuleDef {
 }
 
 impl ModuleDef {
+    /// Construct a `ModuleDef` by validating a `RawModuleDef`.
+    /// This is the only way to construct a `ModuleDef`.
+    /// (The `TryFrom` impls for this type just call this method.)
+    pub fn validate(raw: RawModuleDef) -> Result<Self, ValidationErrors> {
+        match raw {
+            RawModuleDef::V8BackCompat(v8_mod) => validate::v8::validate(v8_mod),
+            RawModuleDef::V9(v9_mod) => validate::v9::validate(v9_mod),
+            _ => unimplemented!(),
+        }
+    }
+
     /// The tables of the module definition.
     pub fn tables(&self) -> impl Iterator<Item = &TableDef> {
         self.tables.values()
@@ -128,7 +139,7 @@ impl ModuleDef {
     /// Generate indexes for the module definition.
     /// We guarantee that all `unique` constraints have an index generated for them.
     /// This will be removed once another enforcement mechanism is implemented.
-    /// We implement this after validation to share logic between v8 and v9.
+    /// This is a noop if there are already usable indexes present.
     fn generate_indexes(&mut self) {
         for table in self.tables.values_mut() {
             for constraint in table.unique_constraints.values() {

crates/schema/src/def.rs

@@ -14,7 +14,10 @@ pub mod tests {
     use spacetimedb_lib::{db::raw_def::v9::RawScopedTypeNameV9, AlgebraicType};
     use spacetimedb_sats::{Typespace, WithTypespace};
 
-    use crate::{def::ScopedTypeName, identifier::Identifier};
+    use crate::{
+        def::{ModuleDef, ScopedTypeName, TableDef},
+        identifier::Identifier,
+    };
 
     /// Create an identifier, panicking if invalid.
     pub fn expect_identifier(data: impl Into<Box<str>>) -> Identifier {
@@ -53,6 +56,22 @@ pub mod tests {
             .expect("failed to resolve type")
     }
 
+    /// Check that the columns of a `TableDef` correctly correspond the the `TableDef`'s
+    /// `product_type_ref`.
+    pub fn check_product_type(module_def: &ModuleDef, table_def: &TableDef) {
+        let product_type = module_def
+            .typespace
+            .get(table_def.product_type_ref)
+            .unwrap()
+            .as_product()
+            .unwrap();
+
+        for (element, column) in product_type.elements.iter().zip(table_def.columns.iter()) {
+            assert_eq!(Some(&*column.name), element.name());
+            assert_eq!(column.ty, element.algebraic_type);
+        }
+    }
+
     #[test]
     fn test_expect_type_name() {
         assert_eq!(