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
Sequence DiagramsequenceDiagram
participant Schema as Database Schema
participant Enum as Enum Type
participant Constraint as CHECK Constraint
Schema->>Enum: Detect Enum Update
alt New Variants Detected
Enum->>Constraint: Drop Existing Constraint
Enum->>Constraint: Create New Constraint
Enum->>Schema: Recursively Add New Columns
else Initial Creation
Enum->>Constraint: Create Initial Constraint
end
The sequence diagram illustrates the new approach to handling enum type updates, showing how the system dynamically manages constraints and schema modifications. 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 55.81% 55.85% +0.03%
==========================================
Files 444 447 +3
Lines 57329 57905 +576
==========================================
+ Hits 32000 32341 +341
- Misses 25329 25564 +235 ☔ 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?
Summary by CodeRabbit