feat(torii-sqlite): support enum upgrade of variants #2930
Conversation
|
Ohayo, sensei! Let me break down the changes for you in our signature style. WalkthroughThe changes focus on enhancing the handling of enum types and struct members in the SQLite database schema management. The Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/sqlite/src/lib.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/sqlite/src/lib.rs
[error] 912-914: Code formatting error: Incorrect line formatting detected. The constraint definition needs to be split across multiple lines using line continuation characters.
🔇 Additional comments (1)
crates/torii/sqlite/src/lib.rs (1)
908-940: Ohayo sensei! The logic for handling enum upgrades looks goodThe implementation correctly updates the
CHECKconstraints and recursively adds columns for new enum variants during an upgrade. This ensures the database schema remains consistent with the updated enum definitions.🧰 Tools
🪛 GitHub Actions: ci
[error] 912-914: Code formatting error: Incorrect line formatting detected. The constraint definition needs to be split across multiple lines using line continuation characters.
crates/torii/sqlite/src/lib.rs
Outdated
| "ALTER TABLE [{table_id}] DROP CONSTRAINT IF EXISTS [{column_name}_check]" | ||
| )); | ||
| alter_table_queries.push(format!( | ||
| "ALTER TABLE [{table_id}] ADD CONSTRAINT [{column_name}_check] CHECK([{column_name}] IN ({all_options}))" |
There was a problem hiding this comment.
Ohayo sensei! Please fix the code formatting in ALTER TABLE statements
The pipeline reported a code formatting error on lines 912-914. The long SQL statement exceeds the maximum line length and needs to be split across multiple lines using line continuation.
Apply this diff to fix the formatting issue:
alter_table_queries.push(format!(
- "ALTER TABLE [{table_id}] ADD CONSTRAINT [{column_name}_check] CHECK([{column_name}] IN ({all_options}))"
+ "ALTER TABLE [{table_id}] ADD CONSTRAINT [{column_name}_check] "
+ "CHECK([{column_name}] IN ({all_options}))"
));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "ALTER TABLE [{table_id}] DROP CONSTRAINT IF EXISTS [{column_name}_check]" | |
| )); | |
| alter_table_queries.push(format!( | |
| "ALTER TABLE [{table_id}] ADD CONSTRAINT [{column_name}_check] CHECK([{column_name}] IN ({all_options}))" | |
| "ALTER TABLE [{table_id}] DROP CONSTRAINT IF EXISTS [{column_name}_check]" | |
| )); | |
| alter_table_queries.push(format!( | |
| "ALTER TABLE [{table_id}] ADD CONSTRAINT [{column_name}_check] " | |
| "CHECK([{column_name}] IN ({all_options}))" | |
| )); |
🧰 Tools
🪛 GitHub Actions: ci
[error] 912-914: Code formatting error: Incorrect line formatting detected. The constraint definition needs to be split across multiple lines using line continuation characters.
Larkooo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/torii/sqlite/src/lib.rs (3)
919-940: Ohayo sensei! Minor code style improvement needed.Remove the unnecessary empty line at 927 to maintain consistent spacing between code blocks.
943-968: Ohayo sensei! Remove unnecessary empty line.Remove the empty line at 955 to maintain consistent spacing.
919-968: Consider reducing code duplication in variant processing.The variant processing logic is duplicated between the upgrade path (lines 919-940) and new table path (lines 943-968). Consider extracting the common logic into a helper function.
Here's a suggested refactor:
+fn process_enum_variants( + e: &Enum, + path: &[String], + columns: &mut Vec<String>, + alter_table_queries: &mut Vec<String>, + indices: &mut Vec<String>, + table_id: &str, + filter_fn: impl Fn(&EnumOption) -> bool, +) -> Result<()> { + for child in &e.options { + if !filter_fn(child) { + continue; + } + + if let Ty::Tuple(tuple) = &child.ty { + if tuple.is_empty() { + continue; + } + } + + let mut new_path = path.to_vec(); + new_path.push(child.name.clone()); + + add_columns_recursive( + &new_path, + &child.ty, + columns, + alter_table_queries, + indices, + table_id, + None, + )?; + } + Ok(()) +}Then use it in both paths:
if let Some(upgrade_diff) = upgrade_diff { if let Ty::Enum(diff_enum) = upgrade_diff { // ... constraint handling ... - for child in &e.options { - if !diff_enum.options.iter().any(|o| o.name == child.name) { - if let Ty::Tuple(tuple) = &child.ty { - if tuple.is_empty() { - continue; - } - } - let mut new_path = path.to_vec(); - new_path.push(child.name.clone()); - add_columns_recursive(/* ... */)?; - } - } + process_enum_variants( + e, + path, + columns, + alter_table_queries, + indices, + table_id, + |child| !diff_enum.options.iter().any(|o| o.name == child.name), + )?; } } else { // ... constraint handling ... - for child in &e.options { - if let Ty::Tuple(tuple) = &child.ty { - if tuple.is_empty() { - continue; - } - } - let mut new_path = path.to_vec(); - new_path.push(child.name.clone()); - add_columns_recursive(/* ... */)?; - } + process_enum_variants( + e, + path, + columns, + alter_table_queries, + indices, + table_id, + |_| true, + )?; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/sqlite/src/lib.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (1)
crates/torii/sqlite/src/lib.rs (1)
908-918: Ohayo sensei! Consider transaction safety for constraint modifications.There's a potential race condition between dropping and adding the CHECK constraint. If a concurrent write occurs between these operations, invalid enum values could be inserted.
Let's verify if transactions are properly handled:
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2930 +/- ##
==========================================
- Coverage 57.10% 57.09% -0.02%
==========================================
Files 424 424
Lines 56215 56277 +62
==========================================
+ Hits 32104 32132 +28
- Misses 24111 24145 +34 ☔ View full report in Codecov by Sentry. |
Larkooo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/torii/sqlite/src/lib.rs (1)
Line range hint
852-877: Ohayo sensei! The struct diff handling looks good!The selective processing of struct members during upgrades is well implemented. It efficiently handles schema evolution by only processing members that exist in the upgrade definition.
Consider adding a debug log when skipping members to help with troubleshooting:
} else { + log::debug!("Skipping member {} as it's not in the upgrade definition", member.name); continue; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/sqlite/src/lib.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: clippy
- GitHub Check: docs
- GitHub Check: build
🔇 Additional comments (2)
crates/torii/sqlite/src/lib.rs (2)
882-906: LGTM! Tuple diff handling is consistent with struct handling.The implementation correctly processes tuple elements during upgrades, maintaining consistency with the struct handling approach.
Line range hint
945-972: Variant diff handling looks good!The selective processing of enum variants during upgrades is well implemented. It efficiently handles schema evolution by only processing new variants that aren't in the original enum.
crates/torii/sqlite/src/lib.rs
Outdated
| if enum_diff.is_some() { | ||
| // For upgrades, modify the existing CHECK constraint to include new options | ||
| alter_table_queries.push(format!( | ||
| "ALTER TABLE [{table_id}] DROP CONSTRAINT IF EXISTS [{column_name}_check]" | ||
| )); | ||
| alter_table_queries.push(format!( | ||
| "ALTER TABLE [{table_id}] ADD CONSTRAINT [{column_name}_check] \ | ||
| CHECK([{column_name}] IN ({all_options}))" | ||
| )); | ||
| } else { | ||
| // For new tables, create the column with explicitly named CHECK constraint | ||
| let sql_type = format!( | ||
| "TEXT CONSTRAINT [{column_name}_check] CHECK([{column_name}] IN \ | ||
| ({all_options}))" | ||
| ); | ||
| add_column(&column_name, &sql_type); | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Please ensure atomic CHECK constraint updates.
The current implementation drops and recreates CHECK constraints separately, which could lead to a brief window where the constraint is not enforced. This might allow invalid enum values to be inserted during schema upgrades.
Consider wrapping the constraint updates in a transaction or using a temporary constraint name:
- alter_table_queries.push(format!(
- "ALTER TABLE [{table_id}] DROP CONSTRAINT IF EXISTS [{column_name}_check]"
- ));
- alter_table_queries.push(format!(
- "ALTER TABLE [{table_id}] ADD CONSTRAINT [{column_name}_check] \
- CHECK([{column_name}] IN ({all_options}))"
- ));
+ // Create new constraint with temporary name
+ alter_table_queries.push(format!(
+ "ALTER TABLE [{table_id}] ADD CONSTRAINT [{column_name}_check_new] \
+ CHECK([{column_name}] IN ({all_options}))"
+ ));
+ // Drop old constraint
+ alter_table_queries.push(format!(
+ "ALTER TABLE [{table_id}] DROP CONSTRAINT IF EXISTS [{column_name}_check]"
+ ));
+ // Rename new constraint
+ alter_table_queries.push(format!(
+ "ALTER TABLE [{table_id}] RENAME CONSTRAINT [{column_name}_check_new] TO [{column_name}_check]"
+ ));Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/torii/sqlite/src/lib.rs (3)
874-887: Ohayo sensei! Enhance the skip comment for better clarity.The comment on line 882 could be more descriptive to explain why we skip members not present in the diff.
Apply this diff to improve the comment:
} else { - // If the member is not in the diff, skip it + // Skip members not present in the upgrade diff to preserve existing columns continue; }
911-920: Ohayo sensei! Optimize tuple diff comparison.The current implementation uses
enumerate().find()which iterates through the diff tuple to find a matching index. Since we already know the index, we can use direct indexing.Apply this diff to optimize the code:
- let member_diff = if let Some(diff) = tuple_diff { - if let Some((_, m)) = diff.iter().enumerate().find(|(i, _)| *i == idx) { - Some(m) - } else { - continue; - } - } else { - None - }; + let member_diff = tuple_diff.and_then(|diff| diff.get(idx)); + if member_diff.is_none() && tuple_diff.is_some() { + continue; + }
952-954: Ohayo sensei! Fix multi-line comment formatting.The comment spans multiple lines incorrectly. It should be properly formatted as a multi-line comment.
Apply this diff to fix the comment:
- // For upgrades, modify the existing option column to add the new options to the - // CHECK constraint We need to drop the old column and create a new - // one with the new CHECK constraint + // For upgrades, modify the existing option column to add the new options to the + // CHECK constraint. We need to drop the old column and create a new one with + // the new CHECK constraint.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/sqlite/src/lib.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: docs
- GitHub Check: ensure-wasm
- GitHub Check: clippy
- GitHub Check: build
| let modify_column = | ||
| |alter_table_queries: &mut Vec<String>, name: &str, sql_type: &str, sql_value: &str| { | ||
| // SQLite doesn't support ALTER COLUMN directly, so we need to: | ||
| // 1. Create a temporary table to store the current values | ||
| // 2. Drop the old column & index | ||
| // 3. Create new column with new type/constraint | ||
| // 4. Copy values back & create new index | ||
| alter_table_queries.push(format!( | ||
| "CREATE TEMPORARY TABLE tmp_values_{name} AS SELECT internal_id, [{name}] FROM \ | ||
| [{table_id}]" | ||
| )); | ||
| alter_table_queries.push(format!("DROP INDEX IF EXISTS [idx_{table_id}_{name}]")); | ||
| alter_table_queries.push(format!("ALTER TABLE [{table_id}] DROP COLUMN [{name}]")); | ||
| alter_table_queries | ||
| .push(format!("ALTER TABLE [{table_id}] ADD COLUMN [{name}] {sql_type}")); | ||
| alter_table_queries.push(format!("UPDATE [{table_id}] SET [{name}] = {sql_value}")); | ||
| alter_table_queries.push(format!("DROP TABLE tmp_values_{name}")); | ||
| alter_table_queries.push(format!( | ||
| "CREATE INDEX IF NOT EXISTS [idx_{table_id}_{name}] ON [{table_id}] ([{name}]);" | ||
| )); | ||
| }; |
There was a problem hiding this comment.
Ohayo sensei! Fix potential data loss in column modification.
The modify_column closure creates a temporary table but doesn't copy the values back to the new column. The UPDATE statement on line 865 sets a static sql_value instead of restoring the original values.
Apply this diff to fix the data loss:
alter_table_queries.push(format!(
"CREATE TEMPORARY TABLE tmp_values_{name} AS SELECT internal_id, [{name}] FROM \
[{table_id}]"
));
alter_table_queries.push(format!("DROP INDEX IF EXISTS [idx_{table_id}_{name}]"));
alter_table_queries.push(format!("ALTER TABLE [{table_id}] DROP COLUMN [{name}]"));
alter_table_queries
.push(format!("ALTER TABLE [{table_id}] ADD COLUMN [{name}] {sql_type}"));
- alter_table_queries.push(format!("UPDATE [{table_id}] SET [{name}] = {sql_value}"));
+ alter_table_queries.push(format!(
+ "UPDATE [{table_id}] SET [{name}] = (SELECT [{name}] FROM tmp_values_{name} \
+ WHERE tmp_values_{name}.internal_id = [{table_id}].internal_id)"
+ ));
alter_table_queries.push(format!("DROP TABLE tmp_values_{name}"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let modify_column = | |
| |alter_table_queries: &mut Vec<String>, name: &str, sql_type: &str, sql_value: &str| { | |
| // SQLite doesn't support ALTER COLUMN directly, so we need to: | |
| // 1. Create a temporary table to store the current values | |
| // 2. Drop the old column & index | |
| // 3. Create new column with new type/constraint | |
| // 4. Copy values back & create new index | |
| alter_table_queries.push(format!( | |
| "CREATE TEMPORARY TABLE tmp_values_{name} AS SELECT internal_id, [{name}] FROM \ | |
| [{table_id}]" | |
| )); | |
| alter_table_queries.push(format!("DROP INDEX IF EXISTS [idx_{table_id}_{name}]")); | |
| alter_table_queries.push(format!("ALTER TABLE [{table_id}] DROP COLUMN [{name}]")); | |
| alter_table_queries | |
| .push(format!("ALTER TABLE [{table_id}] ADD COLUMN [{name}] {sql_type}")); | |
| alter_table_queries.push(format!("UPDATE [{table_id}] SET [{name}] = {sql_value}")); | |
| alter_table_queries.push(format!("DROP TABLE tmp_values_{name}")); | |
| alter_table_queries.push(format!( | |
| "CREATE INDEX IF NOT EXISTS [idx_{table_id}_{name}] ON [{table_id}] ([{name}]);" | |
| )); | |
| }; | |
| let modify_column = | |
| |alter_table_queries: &mut Vec<String>, name: &str, sql_type: &str, sql_value: &str| { | |
| // SQLite doesn't support ALTER COLUMN directly, so we need to: | |
| // 1. Create a temporary table to store the current values | |
| // 2. Drop the old column & index | |
| // 3. Create new column with new type/constraint | |
| // 4. Copy values back & create new index | |
| alter_table_queries.push(format!( | |
| "CREATE TEMPORARY TABLE tmp_values_{name} AS SELECT internal_id, [{name}] FROM \ | |
| [{table_id}]" | |
| )); | |
| alter_table_queries.push(format!("DROP INDEX IF EXISTS [idx_{table_id}_{name}]")); | |
| alter_table_queries.push(format!("ALTER TABLE [{table_id}] DROP COLUMN [{name}]")); | |
| alter_table_queries | |
| .push(format!("ALTER TABLE [{table_id}] ADD COLUMN [{name}] {sql_type}")); | |
| alter_table_queries.push(format!( | |
| "UPDATE [{table_id}] SET [{name}] = (SELECT [{name}] FROM tmp_values_{name} \ | |
| WHERE tmp_values_{name}.internal_id = [{table_id}].internal_id)" | |
| )); | |
| alter_table_queries.push(format!("DROP TABLE tmp_values_{name}")); | |
| alter_table_queries.push(format!( | |
| "CREATE INDEX IF NOT EXISTS [idx_{table_id}_{name}] ON [{table_id}] ([{name}]);" | |
| )); | |
| }; |
crates/torii/sqlite/src/lib.rs
Outdated
| let sql_type = format!( | ||
| "TEXT CONSTRAINT [{column_name}_check] CHECK([{column_name}] IN ({all_options}))" | ||
| ); | ||
| if enum_diff.is_some() { | ||
| // For upgrades, modify the existing option column to add the new options to the | ||
| // CHECK constraint We need to drop the old column and create a new | ||
| // one with the new CHECK constraint | ||
| modify_column( | ||
| alter_table_queries, | ||
| &column_name, | ||
| &sql_type, | ||
| &format!("[{column_name}]"), | ||
| ); | ||
| } else { | ||
| // For new tables, create the column directly | ||
| add_column(&column_name, &sql_type); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Ohayo sensei! The review comment is spot-on about enum variant removal!
The current implementation is indeed unsafe when removing enum variants. The code modifies the CHECK constraint without validating if any rows contain the removed variants, which could lead to invalid data states. The suggested validation using RAISE(ABORT) is essential to fail fast and prevent data inconsistency.
🔗 Analysis chain
Ohayo sensei! Handle removed enum variants.
The current implementation only handles new variants but doesn't account for variants that might have been removed in the upgrade. This could lead to invalid data if variants are removed.
Run this script to check if any enums have removed variants:
Consider adding validation to handle removed variants:
let sql_type = format!(
"TEXT CONSTRAINT [{column_name}_check] CHECK([{column_name}] IN ({all_options}))"
);
if enum_diff.is_some() {
+ // Validate existing data before modifying the constraint
+ alter_table_queries.push(format!(
+ "SELECT CASE WHEN EXISTS (
+ SELECT 1 FROM [{table_id}]
+ WHERE [{column_name}] NOT IN ({all_options})
+ )
+ THEN RAISE(ABORT, 'Found invalid enum variants')
+ END"
+ ));
+
modify_column(
alter_table_queries,
&column_name,🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for removed enum variants in the codebase
# Search for enum definitions and their variants
ast-grep --pattern 'enum $name {
$$$
$variant($$$)
$$$
}'
Length of output: 65
Script:
#!/bin/bash
# Search for enum definitions in Rust
ast-grep --pattern 'enum $name { $$$ }'
# Search for enum-related tests and migration code
rg -l "enum.*diff|enum.*migration" --type rust
# Search for EnumDiff implementation
rg "impl.*EnumDiff" -A 5 --type rust
Length of output: 192
Script:
#!/bin/bash
# Find EnumDiff type definition and usage
rg "struct EnumDiff|type EnumDiff" -A 5 --type rust
# Look for enum migration tests
rg "test.*enum.*migration|test.*enum.*upgrade" -B 2 -A 10 --type rust
# Search for variant removal handling
rg "enum.*remove|variant.*remove|DROP.*CONSTRAINT" -A 5 --type rust
Length of output: 1181
Script:
#!/bin/bash
# Search for CHECK constraint and column modification logic
rg "CHECK.*IN|modify_column|alter_table.*enum" -B 2 -A 5 --type rust
# Look for SQL migration validation
rg "RAISE.*ABORT|SELECT.*EXISTS.*WHERE" -A 5 --type rust
# Find column modification implementation
rg "fn modify_column" -B 2 -A 10 --type rust
Length of output: 1868
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/torii/sqlite/src/lib.rs (2)
850-870:⚠️ Potential issueOhayo sensei! Fix potential data loss in column modification.
The
modify_columnclosure creates a temporary table but doesn't restore the original values. The UPDATE statement on line 865 uses a staticsql_valueinstead of copying data from the temporary table.Apply this diff to fix the data loss:
- alter_table_queries.push(format!("UPDATE [{table_id}] SET [{name}] = {sql_value}")); + alter_table_queries.push(format!( + "UPDATE [{table_id}] SET [{name}] = (SELECT [{name}] FROM tmp_values_{name} \ + WHERE tmp_values_{name}.internal_id = [{table_id}].internal_id)" + ));
948-964:⚠️ Potential issueOhayo sensei! Please ensure atomic CHECK constraint updates.
The current implementation modifies CHECK constraints non-atomically, which could allow invalid enum values during schema upgrades.
Apply this diff to make constraint updates atomic:
- if enum_diff.is_some_and(|diff| diff != e) { - modify_column( - alter_table_queries, - &column_name, - &sql_type, - &format!("[{column_name}]"), - ); + if enum_diff.is_some_and(|diff| diff != e) { + // Create new constraint with temporary name + alter_table_queries.push(format!( + "ALTER TABLE [{table_id}] ADD CONSTRAINT [{column_name}_check_new] \ + CHECK([{column_name}] IN ({all_options}))" + )); + // Drop old constraint + alter_table_queries.push(format!( + "ALTER TABLE [{table_id}] DROP CONSTRAINT IF EXISTS [{column_name}_check]" + )); + // Rename new constraint + alter_table_queries.push(format!( + "ALTER TABLE [{table_id}] RENAME CONSTRAINT [{column_name}_check_new] TO [{column_name}_check]" + ));
🧹 Nitpick comments (1)
crates/torii/sqlite/src/lib.rs (1)
911-920: Ohayo sensei! Optimize tuple index comparison.The tuple member lookup using
findwith index comparison could be replaced with direct indexing since we know the exact position.Apply this diff to optimize the lookup:
- let member_diff = if let Some(diff) = tuple_diff { - if let Some((_, m)) = diff.iter().enumerate().find(|(i, _)| *i == idx) { - Some(m) - } else { - continue; - } + let member_diff = if let Some(diff) = tuple_diff { + diff.get(idx).map(|m| m).or_else(|| { + continue; + })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/sqlite/src/lib.rs(4 hunks)
🔇 Additional comments (1)
crates/torii/sqlite/src/lib.rs (1)
Line range hint
874-899: Ohayo sensei! Add validation for removed struct members.The code skips struct members not present in the upgrade diff but doesn't validate if any existing data uses these removed members. This could lead to orphaned data in the database.
Run this script to check for removed struct members and their usage:
There was a problem hiding this comment.
When trying to upgrade an enum without inner type, this is what I face:
Failed to execute query. type=Other error=Failed to execute query: "ALTER TABLE [ns-Moves] ADD COLUMN [last_direction] TEXT CONSTRAINT [last_direction_check] CHECK([last_direction] IN ('None', 'Left', 'Right', 'Up', 'Down', 'Diag'))", args: [], error: Database(SqliteError { code: 1, message: "duplicate column name: last_direction" })
Adding a variant to:
pub enum Direction {
None,
Left,
Right,
Up,
Down,
Diag, // << this one.
}Can it be because the column hasn't been dropped when this query is executed? May we need to move this logic to the executor with an other query type?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/dojo/types/src/schema.rs (2)
271-281: Ohayo sensei! Align with rustfmt style here.The pipeline indicates that lines 271-279 must be reformatted to match rustfmt guidelines. Ensuring consistent indentation and spacing will help keep the code clean and easier to maintain.
Here's a possible reformat:
.filter_map(|o1| { - if let Some(o2) = e2.options.iter().find(|o2| o2.name == o1.name) { - // Option exists in both - check if types are different - o1.ty.diff(&o2.ty).map(|diff_ty| EnumOption { - name: o1.name.clone(), - ty: diff_ty, - }) - } else { - // Option doesn't exist in e2 - Some(o1.clone()) - } + if let Some(o2) = e2.options.iter().find(|o2| o2.name == o1.name) { + // Option exists in both - check if types are different + o1.ty.diff(&o2.ty).map(|diff_ty| EnumOption { + name: o1.name.clone(), + ty: diff_ty, + }) + } else { + // Option doesn't exist in e2 + Some(o1.clone()) + } })🧰 Tools
🪛 GitHub Actions: ci
[error] 271-279: Code formatting does not match rustfmt style. The EnumOption construction needs to be reformatted to use method chaining style.
313-317: Ohayo sensei! Consider partial diffs for arrays.Right now, the logic returns the entire array from
a1whenevera1 != a2. If only some elements differ, you lose any nuanced diffs. If the intent is to fully replace arrays when there’s any difference, this is fine. Otherwise, partial diffing might be more flexible.Example approach:
// For each pair of elements, compare them for differences let diff_items = a1 .iter() .zip(a2.iter()) .filter_map(|(ty1, ty2)| ty1.diff(ty2)) .collect::<Vec<_>>(); if !diff_items.is_empty() { Some(Ty::Array(diff_items)) } else { None }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/dojo/types/src/schema.rs(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/dojo/types/src/schema.rs
[error] 271-279: Code formatting does not match rustfmt style. The EnumOption construction needs to be reformatted to use method chaining style.
[error] 297-309: Code formatting does not match rustfmt style. The tuple diff_elements construction and conditional expression need to be reformatted to use more concise syntax.
🔇 Additional comments (1)
crates/dojo/types/src/schema.rs (1)
295-310: Ohayo sensei! Verify ignoring the comparison tot2when lengths differ, plus align with rustfmt.When
t1.len() != t2.len(), the code simply returnsSome(Ty::Tuple(t1.clone())), ignoringt2entirely. Confirm that this is indeed the intended behavior. Additionally, lines 297-309 have formatting issues flagged by the pipeline, so reformatting is also needed.[verify, nitpick]
Below is an example snippet to satisfy the style guide and possibly clarify the logic:
if t1.len() != t2.len() { - Some(Ty::Tuple(t1.clone())) -} else { - // Compare each tuple element recursively - let diff_elements: Vec<Ty> = t1 - .iter() - .zip(t2.iter()) - .filter_map(|(ty1, ty2)| ty1.diff(ty2)) - .collect(); - if diff_elements.is_empty() { - None - } else { - Some(Ty::Tuple(diff_elements)) - } + Some(Ty::Tuple(t1.clone())) } else { + // Compare each tuple element recursively + let diff_elements: Vec<Ty> = t1.iter() + .zip(t2.iter()) + .filter_map(|(ty1, ty2)| ty1.diff(ty2)) + .collect(); + + if diff_elements.is_empty() { + None + } else { + Some(Ty::Tuple(diff_elements)) + } }🧰 Tools
🪛 GitHub Actions: ci
[error] 297-309: Code formatting does not match rustfmt style. The tuple diff_elements construction and conditional expression need to be reformatted to use more concise syntax.
| .filter_map(|m1| { | ||
| if let Some(m2) = s2.children.iter().find(|m2| m2.name == m1.name) { | ||
| // Member exists in both - check if types are different | ||
| m1.ty.diff(&m2.ty).map(|diff_ty| Member { | ||
| name: m1.name.clone(), | ||
| ty: diff_ty, | ||
| key: m1.key, | ||
| }) | ||
| } else { | ||
| // Member doesn't exist in s2 | ||
| Some(m1.clone()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Consider diffing changes to the key property.
Currently, this logic only compares and diffs member types. If a member changes from non-key to key (or vice versa), it won’t be reflected. This might lead to schema discrepancies where primary keys are misaligned.
Here's a small illustrative snippet to demonstrate checking for key changes:
if let Some(m2) = s2.children.iter().find(|m2| m2.name == m1.name) {
// Member exists in both - check for differences
// ...
- m1.ty.diff(&m2.ty).map(|diff_ty| Member {
+ let diff_ty = m1.ty.diff(&m2.ty);
+ let diff_key = (m1.key != m2.key);
+ if diff_ty.is_some() || diff_key {
// Return a new member capturing both potential type & key changes
Some(Member {
name: m1.name.clone(),
ty: diff_ty.unwrap_or_else(|| m1.ty.clone()),
key: m1.key,
})
} else {
None
}
} else {
Some(m1.clone())
}Committable suggestion skipped: line range outside the PR's diff.
Ok should be fixed |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/dojo/types/src/schema.rs (1)
305-309: Consider implementing a more granular array diff.The current implementation returns the entire array when any element differs. For large arrays, a more granular diff (similar to the tuple implementation) could be more efficient.
Apply this diff for a more granular implementation:
(Ty::Array(a1), Ty::Array(a2)) => { - if a1 == a2 { - None - } else { - Some(Ty::Array(a1.clone())) + if a1.len() != a2.len() { + Some(Ty::Array(a1.clone())) + } else { + let diff_elements: Vec<Ty> = a1 + .iter() + .zip(a2.iter()) + .filter_map(|(ty1, ty2)| ty1.diff(ty2)) + .collect(); + if diff_elements.is_empty() { + None + } else { + Some(Ty::Array(diff_elements)) + } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/dojo/types/src/schema.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (3)
crates/dojo/types/src/schema.rs (3)
271-280: Ohayo sensei! The enum diff implementation looks solid!The implementation correctly handles both cases where enum options differ in type or existence, which should resolve the enum upgrade issues mentioned in the PR objectives.
294-302: Ohayo sensei! The tuple diff implementation is elegant!The implementation efficiently handles both length and element differences using
zipfor element-wise comparison.
245-256: 🛠️ Refactor suggestionOhayo sensei! Consider diffing changes to the
keyproperty.The current implementation only compares member types. If a member changes from non-key to key (or vice versa), it won't be reflected in the diff.
Apply this diff to include key property comparison:
if let Some(m2) = s2.children.iter().find(|m2| m2.name == m1.name) { // Member exists in both - check if types are different - m1.ty.diff(&m2.ty).map(|diff_ty| Member { + let diff_ty = m1.ty.diff(&m2.ty); + let key_changed = m1.key != m2.key; + if diff_ty.is_some() || key_changed { + Some(Member { name: m1.name.clone(), - ty: diff_ty, + ty: diff_ty.unwrap_or_else(|| m1.ty.clone()), key: m1.key, - }) + }) + } else { + None + } } else { // Member doesn't exist in s2 Some(m1.clone()) }Likely invalid or redundant comment.
There was a problem hiding this comment.
Works great on most cases, thank you for the rework @Larkooo!
I still have an issue when working on a tuple, upgrading an existing variant by widening one of the tuple member, I face this:
Failed to execute query. type=Other error=Failed to execute query: "ALTER TABLE [ns-Moved] ADD COLUMN [direction.Gra.0] INTEGER", args: [], error: Database(SqliteError { code: 1, message: "duplicate column name: direction.Gra.0" })
Oh I thought we didnt support tuple upgrades. Will update |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/dojo/types/src/schema.rs(3 hunks)crates/torii/sqlite/src/lib.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/dojo/types/src/schema.rs
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/sqlite/src/lib.rs
[error] 901-904: Code formatting error: Closure needs to be formatted with proper line breaks and indentation
🔇 Additional comments (4)
crates/torii/sqlite/src/lib.rs (4)
872-885: Ohayo sensei! Clean and efficient struct diff handling.The struct upgrade logic efficiently handles member differences by:
- Extracting struct diffs from upgrade_diff
- Skipping members not present in the diff
- Properly handling member type differences
934-959: Ohayo sensei! Robust enum upgrade handling.The enum upgrade logic effectively handles:
- Enum diff extraction
- CHECK constraint modifications
- Variant differences
Line range hint
962-989: Ohayo sensei! Clean variant diff handling.The variant upgrade logic efficiently handles:
- Processing only new variants
- Skipping empty tuple variants
- Proper variant type differences
848-868:⚠️ Potential issueOhayo sensei! Fix potential data loss in column modification.
The
modify_columnclosure creates a temporary table but doesn't restore the original values. The UPDATE statement on line 863 uses a staticsql_valueinstead of copying data from the temporary table.Apply this diff to fix the data loss:
- alter_table_queries.push(format!("UPDATE [{table_id}] SET [{name}] = {sql_value}")); + alter_table_queries.push(format!( + "UPDATE [{table_id}] SET [{name}] = (SELECT [{name}] FROM tmp_values_{name} \ + WHERE tmp_values_{name}.internal_id = [{table_id}].internal_id)" + ));Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
crates/torii/sqlite/src/lib.rs (1)
Line range hint
939-994: Ohayo sensei! Add validation for enum upgrades.The enum upgrade handling should validate existing data before modifying CHECK constraints to prevent invalid states.
Add validation before modifying constraints:
if enum_diff.is_some_and(|diff| diff != e) { + // Validate existing data before modifying constraints + alter_table_queries.push(format!( + "SELECT CASE WHEN EXISTS ( + SELECT 1 FROM [{table_id}] + WHERE [{column_name}] NOT IN ({all_options}) + ) + THEN RAISE(ABORT, 'Found invalid enum variants') + END" + )); + // For upgrades, modify the existing option column modify_column( alter_table_queries, &column_name, &sql_type, &format!("[{column_name}]"), );
🧹 Nitpick comments (1)
crates/torii/sqlite/src/lib.rs (1)
902-916: Ohayo sensei! Consider improving tuple diff readability.The tuple diff handling logic is correct but could be more readable.
Consider this more explicit approach:
- let elements_to_process = if let Some(diff) = upgrade_diff.and_then(|d| d.as_tuple()) { - // Only process elements from the diff - diff.iter() - .filter_map(|m| { - tuple.iter().position(|member| member == m).map(|idx| (idx, m, Some(m))) - }) - .collect() + let elements_to_process = if let Some(diff) = upgrade_diff.and_then(|d| d.as_tuple()) { + // Only process elements from the diff + let mut elements = Vec::new(); + for diff_member in diff.iter() { + if let Some(idx) = tuple.iter().position(|member| member == diff_member) { + elements.push((idx, diff_member, Some(diff_member))); + } + } + elements
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/dojo/types/src/schema.rs(3 hunks)crates/torii/sqlite/src/lib.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/dojo/types/src/schema.rs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: docs
- GitHub Check: clippy
- GitHub Check: build
- GitHub Check: ensure-wasm
🔇 Additional comments (2)
crates/torii/sqlite/src/lib.rs (2)
872-885: Ohayo sensei! The struct diff handling looks good!The implementation correctly handles struct member upgrades by:
- Checking for differences between current and upgraded definitions
- Skipping members not present in the upgrade diff
- Passing member-specific diffs for recursive processing
848-868:⚠️ Potential issueOhayo sensei! Fix potential data loss in column modification.
The
modify_columnclosure creates a temporary table but doesn't copy the values back to the new column. TheUPDATEstatement on line 863 sets a staticsql_valueinstead of restoring the original values.Apply this diff to fix the data loss:
alter_table_queries.push(format!( "CREATE TEMPORARY TABLE tmp_values_{name} AS SELECT internal_id, [{name}] FROM \ [{table_id}]" )); alter_table_queries.push(format!("DROP INDEX IF EXISTS [idx_{table_id}_{name}]")); alter_table_queries.push(format!("ALTER TABLE [{table_id}] DROP COLUMN [{name}]")); alter_table_queries .push(format!("ALTER TABLE [{table_id}] ADD COLUMN [{name}] {sql_type}")); - alter_table_queries.push(format!("UPDATE [{table_id}] SET [{name}] = {sql_value}")); + alter_table_queries.push(format!( + "UPDATE [{table_id}] SET [{name}] = (SELECT [{name}] FROM tmp_values_{name} \ + WHERE tmp_values_{name}.internal_id = [{table_id}].internal_id)" + )); alter_table_queries.push(format!("DROP TABLE tmp_values_{name}"));Likely invalid or redundant comment.
Summary by CodeRabbit
StructandEnumtypes in the schema.Tupletypes in the difference identification process.