Add delete_by_{nonunique_column} method to #[spacetimedb(table)] (#859)

- Refactor `bindings::query::delete_by_field` to require `FilterableValue`
  and return `u32` count of deleted rows.
- Add `bindings::query::delete_by_unique_field`
  with the previous behavior of `delete_by_field`,
  i.e. requiring `UniqueValue` and returning `bool` with `true = deleted`.
  This is implemented in terms of `delete_by_field`.
- Macro-generated unique filter funcs call `delete_by_unique_field`
  rather than `delete_by_field`.
- The table macro now generates non-unique delete functions
  alongside the existing non-unique filter functions
  for non-unique fields with primitive types.
- Add uses of `::delete_by_{field}` for both unique and non-unique columns
  to the `rust-wasm-test` module.

Note that this PR does not include any host-side changes;
the diff is localized to the `bindings` and `bindings-macro` crates.

Author: gefjon

crates/bindings-macro/src/lib.rs

@@ -633,7 +633,7 @@ fn spacetimedb_tabletype_impl(item: syn::DeriveInput) -> syn::Result<TokenStream
 
         unique_delete_funcs.push(quote! {
             #vis fn #delete_func_ident(#column_ident: &#column_type) -> bool {
-                spacetimedb::query::delete_by_field::<Self, #column_type, #column_index>(#column_ident)
+                spacetimedb::query::delete_by_unique_field::<Self, #column_type, #column_index>(#column_ident)
             }
         });
     }
@@ -645,6 +645,7 @@ fn spacetimedb_tabletype_impl(item: syn::DeriveInput) -> syn::Result<TokenStream
         let column_index = column.index;
 
         let filter_func_ident = format_ident!("filter_by_{}", column_ident);
+        let delete_func_ident = format_ident!("delete_by_{}", column_ident);
 
         let skip = if let syn::Type::Path(p) = column_type {
             // TODO: this is janky as heck
@@ -665,6 +666,9 @@ fn spacetimedb_tabletype_impl(item: syn::DeriveInput) -> syn::Result<TokenStream
             #vis fn #filter_func_ident<'a>(#column_ident: &#column_type) -> impl Iterator<Item = Self> {
                 spacetimedb::query::filter_by_field::<Self, #column_type, #column_index>(#column_ident)
             }
+            #vis fn #delete_func_ident(#column_ident: &#column_type) -> u32 {
+                spacetimedb::query::delete_by_field::<Self, #column_type, #column_index>(#column_ident)
+            }
         })
     });
     let non_primary_filter_func = non_primary_filter_func.collect::<Vec<_>>();

crates/bindings/src/lib.rs

@@ -505,30 +505,36 @@ pub mod query {
         }
     }
 
+    /// Deletes all rows of `Table` where the column at `COL_IDX` matches `val`,
+    /// as defined by decoding to an `AlgebraicValue`
+    /// according to the column's schema and then `Ord for AlgebraicValue`.
+    ///
+    /// Returns the number of deleted rows.
+    ///
+    /// **NOTE:** Do not use directly.
+    /// This is exposed as `delete_by_{$field_name}` on types with `#[spacetimedb(table)]`
+    /// where the field does not have a unique constraint.
+    #[doc(hidden)]
+    pub fn delete_by_field<Table: TableType, T: FilterableValue, const COL_IDX: u8>(val: &T) -> u32 {
+        delete_by_col_eq(Table::table_id(), COL_IDX, val)
+            // TODO: Returning `Err` here was supposed to signify an error,
+            //       but it can also return `Err(_)` when there is nothing to delete.
+            .unwrap_or(0)
+    }
+
     /// Deletes the row of `Table` where the column at `COL_IDX` matches `val`,
     /// as defined by decoding to an `AlgebraicValue`
     /// according to the column's schema and then `Ord for AlgebraicValue`.
     ///
     /// Returns whether any rows were deleted.
     ///
     /// **NOTE:** Do not use directly.
-    /// This is exposed as `delete_by_{$field_name}` on types with `#[spacetimedb(table)]`.
-    #[doc(hidden)]
-    pub fn delete_by_field<Table: TableType, T: UniqueValue, const COL_IDX: u8>(val: &T) -> bool {
-        let result = delete_by_col_eq(Table::table_id(), COL_IDX, val);
-        match result {
-            Err(_) => {
-                // TODO: Returning here was supposed to signify an error,
-                //       but it can also return `Err(_)` when there is nothing to delete.
-                //spacetimedb::println!("Internal server error on equatable type: {}", #primary_key_tuple_type_str);
-                false
-            }
-            // Should never be `> 1`.
-            Ok(count) => {
-                debug_assert!(count <= 1);
-                count > 0
-            }
-        }
+    /// This is exposed as `delete_by_{$field_name}` on types with `#[spacetimedb(table)]`
+    /// where the field has a unique constraint.
+    pub fn delete_by_unique_field<Table: TableType, T: UniqueValue, const COL_IDX: u8>(val: &T) -> bool {
+        let count = delete_by_field::<Table, T, COL_IDX>(val);
+        debug_assert!(count <= 1);
+        count > 0
     }
 
     /// Updates the row of `Table`, where the column at `COL_IDX` matches `old`, to be `new` instead.

modules/rust-wasm-test/src/lib.rs

@@ -1,8 +1,5 @@
 #![allow(clippy::disallowed_names)]
-use spacetimedb::{
-    delete_by_col_eq, query, spacetimedb, AlgebraicValue, Deserialize, ReducerContext, SpacetimeType, TableType,
-    Timestamp,
-};
+use spacetimedb::{query, spacetimedb, Deserialize, ReducerContext, SpacetimeType, Timestamp};
 use spacetimedb_lib::bsatn;
 
 #[spacetimedb(table)]
@@ -94,7 +91,6 @@ pub fn test(ctx: ReducerContext, arg: TestAlias, arg2: TestB, arg3: TestC) -> an
         TestC::Foo => log::info!("Foo"),
         TestC::Bar => log::info!("Bar"),
     }
-    let table_id = TestA::table_id();
     for i in 0..1000 {
         TestA::insert(TestA {
             x: i + arg.x,
@@ -103,15 +99,25 @@ pub fn test(ctx: ReducerContext, arg: TestAlias, arg2: TestB, arg3: TestC) -> an
         });
     }
 
-    let row_count = TestA::iter().count();
+    let row_count_before_delete = TestA::iter().count();
 
-    log::info!("Row count before delete: {:?}", row_count);
+    log::info!("Row count before delete: {:?}", row_count_before_delete);
 
+    let mut num_deleted = 0;
     for row in 5..10 {
-        delete_by_col_eq(table_id, 0, &AlgebraicValue::U32(row))?;
+        num_deleted += TestA::delete_by_x(&row);
     }
 
-    let row_count = TestA::iter().count();
+    let row_count_after_delete = TestA::iter().count();
+
+    if row_count_before_delete != row_count_after_delete + num_deleted as usize {
+        log::error!(
+            "Started with {} rows, deleted {}, and wound up with {} rows... huh?",
+            row_count_before_delete,
+            num_deleted,
+            row_count_after_delete,
+        );
+    }
 
     match TestE::insert(TestE {
         id: 0,
@@ -121,7 +127,7 @@ pub fn test(ctx: ReducerContext, arg: TestAlias, arg2: TestB, arg3: TestC) -> an
         Err(err) => log::info!("Error: {:?}", err),
     }
 
-    log::info!("Row count after delete: {:?}", row_count);
+    log::info!("Row count after delete: {:?}", row_count_after_delete);
 
     let other_row_count = query!(|row: TestA| row.x >= 0 && row.x <= u32::MAX).count();
 
@@ -150,6 +156,26 @@ pub fn add_player(name: String) -> Result<(), String> {
     Ok(())
 }
 
+#[spacetimedb(reducer)]
+pub fn delete_player(id: u64) -> Result<(), String> {
+    if TestE::delete_by_id(&id) {
+        Ok(())
+    } else {
+        Err(format!("No TestE row with id {}", id))
+    }
+}
+
+#[spacetimedb(reducer)]
+pub fn delete_players_by_name(name: String) -> Result<(), String> {
+    match TestE::delete_by_name(&name) {
+        0 => Err(format!("No TestE row with name {:?}", name)),
+        num_deleted => {
+            log::info!("Deleted {} player(s) with name {:?}", num_deleted, name);
+            Ok(())
+        }
+    }
+}
+
 #[spacetimedb(connect)]
 fn on_connect(_ctx: ReducerContext) {}