Skip to content

Commit

Permalink
This fixes an issue where we were using __ to validate index names er…
Browse files Browse the repository at this point in the history 
…roneously
  • Loading branch information
@cloutiertyler
cloutiertyler committed Dec 9, 2023
1 parent f8296c0 commit a98c989
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 29 deletions.
2 changes: 1 addition & 1 deletion crates/cli/src/subcommands/generate/csharp.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ pub fn autogen_csharp_table(ctx: &GenCtx, table: &TableDesc, namespace: &str) ->
.clone()
.into_schema(0.into())
.validated()
.expect("Fail to generate table"),
.expect("Failed to generate table due to validation errors")
),
namespace,
)
Expand Down
3 changes: 2 additions & 1 deletion crates/cli/src/subcommands/generate/rust.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,7 @@ fn find_product_type(ctx: &GenCtx, ty: AlgebraicTypeRef) -> &ProductType {
/// Generate a file which defines a `struct` corresponding to the `table`'s `ProductType`,
/// and implements `spacetimedb_sdk::table::TableType` for it.
pub fn autogen_rust_table(ctx: &GenCtx, table: &TableDesc) -> String {
println!("{:?}", table);
let mut output = CodeIndenter::new(String::new());
let out = &mut output;

Expand All @@ -347,7 +348,7 @@ pub fn autogen_rust_table(ctx: &GenCtx, table: &TableDesc) -> String {
.clone()
.into_schema(0.into())
.validated()
.expect("Fail to generate table");
.expect("Failed to generate table due to validation errors");
print_impl_tabletype(ctx, out, &table);

output.into_inner()
Expand Down
88 changes: 61 additions & 27 deletions crates/sats/src/db/def.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -836,7 +836,7 @@ impl TableSchema {
}));

errors.extend(self.indexes.iter().filter_map(|x| {
if x.index_name.is_empty() || x.index_name.contains("__") {
if x.index_name.is_empty() {
Some(SchemaError::EmptyName {
table: self.table_name.clone(),
ty: DefType::Index,
Expand All @@ -861,7 +861,7 @@ impl TableSchema {
}));

errors.extend(self.constraints.iter().filter_map(|x| {
if x.constraint_name.is_empty() || x.constraint_name.contains("__") {
if x.constraint_name.is_empty() {
Some(SchemaError::EmptyName {
table: self.table_name.clone(),
ty: DefType::Constraint,
Expand All @@ -873,7 +873,7 @@ impl TableSchema {
}));

errors.extend(self.sequences.iter().filter_map(|x| {
if x.sequence_name.is_empty() || x.sequence_name.contains("__") {
if x.sequence_name.is_empty() {
Some(SchemaError::EmptyName {
table: self.table_name.clone(),
ty: DefType::Sequence,
Expand Down Expand Up @@ -1026,7 +1026,9 @@ impl TableDef {

/// Concatenate the column names from the `columns`
///
/// WARNING: If the `ColId` not exist, is skipped. [TableSchema::validated] will find the error
/// WARNING: If the `ColId` not exist, is skipped.
/// TODO(Tyler): This should return an error and not allow this to be constructed
/// if there is an invalid `ColId`
fn generate_cols_name(&self, columns: &NonEmpty<ColId>) -> String {
let mut column_name = Vec::with_capacity(columns.len());
for col_pos in columns {
Expand Down Expand Up @@ -1392,20 +1394,6 @@ mod tests {
fn test_validate_empty() {
let t = table_def();

fn empty(table: TableDef, ty: &[DefType], id: u32) {
assert_eq!(
table.into_schema(TableId(0)).validated(),
Err(ty
.iter()
.copied()
.map(|ty| SchemaError::EmptyName {
table: "test".to_string(),
ty,
id
})
.collect())
)
}
// Empty names
let mut t_name = t.clone();
t_name.table_name.clear();
Expand All @@ -1416,20 +1404,66 @@ mod tests {

let mut t_col = t.clone();
t_col.columns.push(ColumnDef::sys("", AlgebraicType::U64));
empty(t_col, &[DefType::Column], 5);
assert_eq!(
t_col.into_schema(TableId(0)).validated(),
Err(vec![
SchemaError::EmptyName {
table: "test".to_string(),
ty: DefType::Column,
id: 5
},
])
);

let mut t_ct = t.clone();
t_ct.constraints
.push(ConstraintDef::new("".into(), Constraints::primary_key(), ColId(0)));
empty(t_ct, &[DefType::Index, DefType::Constraint], 0);

let mut t_idx = t.clone();
t_idx.indexes.push(IndexDef::for_column("", "", ColId(0), false));
empty(t_idx, &[DefType::Index, DefType::Constraint], 0);
assert_eq!(
t_ct.into_schema(TableId(0)).validated(),
Err(vec![
SchemaError::EmptyName {
table: "test".to_string(),
ty: DefType::Constraint,
id: 0,
},
])
);

let mut t_seq = t.clone();
t_seq.sequences.push(SequenceDef::for_column("", "", ColId(0)));
empty(t_seq, &[DefType::Sequence], 0);
// TODO(Tyler): I am disabling this because it's actually not correct.
// Previously Mario was checking for __ to see if the name was missing from the
// column, but it's completely valid for an index to have __ in the name.
// This will have to be checked another way.
//
// let mut t_idx = t.clone();
// t_idx.indexes.push(IndexDef::for_column("", "", ColId(0), false));
// assert_eq!(
// t_idx.into_schema(TableId(0)).validated(),
// Err(vec![
// SchemaError::EmptyName {
// table: "test".to_string(),
// ty: DefType::Index,
// id: 0,
// },
// SchemaError::EmptyName {
// table: "test".to_string(),
// ty: DefType::Constraint,
// id: 0,
// },
// ])
// );
//
// let mut t_seq = t.clone();
// t_seq.sequences.push(SequenceDef::for_column("", "", ColId(0)));
// assert_eq!(
// t_seq.into_schema(TableId(0)).validated(),
// Err(vec![
// SchemaError::EmptyName {
// table: "test".to_string(),
// ty: DefType::Sequence,
// id: 0,
// },
// ])
// );
}

// Verify only one PK
Expand Down

0 comments on commit a98c989

Please sign in to comment.