feat(torii-sqlite): primitive upgrades towards bigger types #2931
Conversation
WalkthroughOhayo, sensei! The changes enhance the SQLite database schema upgrade mechanism by introducing a new Changes
Sequence DiagramsequenceDiagram
participant Schema as Database Schema
participant Modifier as Column Modifier
participant Primitive as Type Checker
Primitive->>Primitive: can_upgrade_to()
alt Upgrade Possible
Modifier->>Schema: Create Temporary Column
Modifier->>Schema: Copy Data
Modifier->>Schema: Drop Original Column
Modifier->>Schema: Rename Temporary Column
else Upgrade Not Possible
Primitive-->>Modifier: Return Error
end
🪧 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
🧹 Nitpick comments (1)
crates/torii/sqlite/src/lib.rs (1)
957-977: Ohayo, sensei! Enhance error handling for invalid primitive type upgrades.The error message in lines 967-971 could be more informative to aid debugging:
return Err(anyhow::anyhow!( "Invalid primitive type upgrade from {:?} to {:?}", old_primitive, p ));Consider including additional context or guidance on how to resolve the error. Additionally, logging the error might help in monitoring and troubleshooting.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/sqlite/src/lib.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (1)
crates/torii/sqlite/src/lib.rs (1)
983-1020: Ohayo, sensei! Thecan_upgrade_tomethod correctly implements type upgrade logic.The implementation accurately defines the permissible upgrades between primitive types, ensuring data integrity during schema migrations.
crates/torii/sqlite/src/lib.rs
Outdated
| let mut modify_column = |name: &str, sql_type: &str, sql_value: &str| { | ||
| // SQLite doesn't support ALTER COLUMN directly, so we need to: | ||
| // 1. Create a new temporary column | ||
| // 2. Copy the data | ||
| // 3. Drop the old column | ||
| // 4. Rename the temporary column | ||
| alter_table_queries | ||
| .push(format!("ALTER TABLE [{table_id}] ADD COLUMN [tmp_{name}] {sql_type}")); | ||
| alter_table_queries.push(format!("UPDATE [{table_id}] SET [tmp_{name}] = {sql_value}")); | ||
| alter_table_queries.push(format!("ALTER TABLE [{table_id}] DROP COLUMN [{name}]")); | ||
| alter_table_queries | ||
| .push(format!("ALTER TABLE [{table_id}] RENAME COLUMN [tmp_{name}] TO [{name}]")); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Ohayo, sensei! Address potential SQL injection vulnerability in modify_column.
In the modify_column closure, the sql_value is directly interpolated into the SQL statement without parameterization:
alter_table_queries.push(format!("UPDATE [{table_id}] SET [tmp_{name}] = {sql_value}"));This approach might expose the code to SQL injection attacks if sql_value contains malicious input. It's essential to use parameterized queries or ensure that sql_value is properly sanitized before use.
Apply this diff to fix the issue by using parameterized queries:
- alter_table_queries.push(format!("UPDATE [{table_id}] SET [tmp_{name}] = {sql_value}"));
+ alter_table_queries.push(format!("UPDATE [{table_id}] SET [tmp_{name}] = ?"));
+ // Ensure to pass `sql_value` as a parameter when executing the query.
Larkooo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/torii/sqlite/src/lib.rs (1)
958-979: Enhance error message and add logging for upgrades.The error message could be more descriptive, and successful upgrades should be logged for better observability.
Apply this diff to improve the error handling and add logging:
if old_primitive.can_upgrade_to(p) { + tracing::info!( + "Upgrading column {} from {:?} to {:?}", + column_name, old_primitive, p + ); modify_column( alter_table_queries, &column_name, p.to_sql_type().as_ref(), p.to_sql_value().as_ref(), ); } else { return Err(anyhow::anyhow!( - "Invalid primitive type upgrade from {:?} to {:?}", + "Invalid primitive type upgrade for column '{}': cannot upgrade from {:?} to {:?}. Please check the upgrade compatibility rules.", + column_name, old_primitive, p )); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/dojo/types/src/primitive.rs(1 hunks)crates/torii/sqlite/src/lib.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ensure-wasm
- GitHub Check: docs
- GitHub Check: build
🔇 Additional comments (1)
crates/dojo/types/src/primitive.rs (1)
407-441: Ohayo, sensei! Add test coverage for the newcan_upgrade_tomethod.The new method defines critical upgrade paths for primitive types but lacks test coverage to ensure the upgrade rules are working as expected.
Would you like me to generate comprehensive test cases covering all upgrade paths?
| (U8(_), U16(_) | U32(_) | USize(_) | U64(_) | U128(_) | Felt252(_)) => true, | ||
| (U16(_), U32(_) | USize(_) | U64(_) | U128(_) | Felt252(_)) => true, | ||
| (U32(_), USize(_) | U64(_) | U128(_) | Felt252(_)) => true, | ||
| (USize(_), U32(_) | U64(_) | U128(_) | Felt252(_)) => true, |
There was a problem hiding this comment.
Verify USize upgrade path to prevent data loss.
The USize type can upgrade to U32 which might cause data loss on 64-bit systems where usize is 64 bits.
Apply this diff to fix the upgrade path:
- (USize(_), U32(_) | U64(_) | U128(_) | Felt252(_)) => true,
+ (USize(_), U64(_) | U128(_) | Felt252(_)) => true,📝 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.
| (USize(_), U32(_) | U64(_) | U128(_) | Felt252(_)) => true, | |
| (USize(_), U64(_) | U128(_) | Felt252(_)) => true, |
| 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 new temporary column | ||
| // 2. Copy the data | ||
| // 3. Drop the old column | ||
| // 4. Rename the temporary column | ||
| alter_table_queries | ||
| .push(format!("ALTER TABLE [{table_id}] ADD COLUMN [tmp_{name}] {sql_type}")); | ||
| alter_table_queries.push(format!("UPDATE [{table_id}] SET [tmp_{name}] = {sql_value}")); | ||
| alter_table_queries.push(format!("ALTER TABLE [{table_id}] DROP COLUMN [{name}]")); | ||
| alter_table_queries | ||
| .push(format!("ALTER TABLE [{table_id}] RENAME COLUMN [tmp_{name}] TO [{name}]")); | ||
| }; |
There was a problem hiding this comment.
Ensure atomic operations with transaction handling.
The column modification process involves multiple SQL operations that should be atomic to prevent data corruption if any step fails.
Wrap the operations in a transaction by adding these queries at the start and end:
+ alter_table_queries.push("BEGIN TRANSACTION;".to_string());
alter_table_queries
.push(format!("ALTER TABLE [{table_id}] ADD COLUMN [tmp_{name}] {sql_type}"));
alter_table_queries.push(format!("UPDATE [{table_id}] SET [tmp_{name}] = {sql_value}"));
alter_table_queries.push(format!("ALTER TABLE [{table_id}] DROP COLUMN [{name}]"));
alter_table_queries
.push(format!("ALTER TABLE [{table_id}] RENAME COLUMN [tmp_{name}] TO [{name}]"));
+ alter_table_queries.push("COMMIT;".to_string());Also, add error handling:
+ alter_table_queries.push("ROLLBACK;".to_string());📝 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 new temporary column | |
| // 2. Copy the data | |
| // 3. Drop the old column | |
| // 4. Rename the temporary column | |
| alter_table_queries | |
| .push(format!("ALTER TABLE [{table_id}] ADD COLUMN [tmp_{name}] {sql_type}")); | |
| alter_table_queries.push(format!("UPDATE [{table_id}] SET [tmp_{name}] = {sql_value}")); | |
| alter_table_queries.push(format!("ALTER TABLE [{table_id}] DROP COLUMN [{name}]")); | |
| alter_table_queries | |
| .push(format!("ALTER TABLE [{table_id}] RENAME COLUMN [tmp_{name}] TO [{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 new temporary column | |
| // 2. Copy the data | |
| // 3. Drop the old column | |
| // 4. Rename the temporary column | |
| alter_table_queries.push("BEGIN TRANSACTION;".to_string()); | |
| alter_table_queries | |
| .push(format!("ALTER TABLE [{table_id}] ADD COLUMN [tmp_{name}] {sql_type}")); | |
| alter_table_queries.push(format!("UPDATE [{table_id}] SET [tmp_{name}] = {sql_value}")); | |
| alter_table_queries.push(format!("ALTER TABLE [{table_id}] DROP COLUMN [{name}]")); | |
| alter_table_queries | |
| .push(format!("ALTER TABLE [{table_id}] RENAME COLUMN [tmp_{name}] TO [{name}]")); | |
| alter_table_queries.push("COMMIT;".to_string()); | |
| alter_table_queries.push("ROLLBACK;".to_string()); | |
| }; |
| // Felt252 and address types | ||
| (Felt252(_), ClassHash(_) | ContractAddress(_)) => true, | ||
| (ClassHash(_), Felt252(_) | ContractAddress(_)) => true, | ||
| (ContractAddress(_), Felt252(_) | ClassHash(_)) => true, |
There was a problem hiding this comment.
We're missing EthAddress, not sure at which point it's relevant for now and may be supported later.
| (U8(_), U16(_) | U32(_) | USize(_) | U64(_) | U128(_) | Felt252(_)) => true, | ||
| (U16(_), U32(_) | USize(_) | U64(_) | U128(_) | Felt252(_)) => true, | ||
| (U32(_), USize(_) | U64(_) | U128(_) | Felt252(_)) => true, | ||
| (USize(_), U32(_) | U64(_) | U128(_) | Felt252(_)) => true, |
There was a problem hiding this comment.
I've left a comment here #2925 (comment), I think having usize non-changeable would make more sense at the cairo level (for Torii, it's way less relevant, but it is to match the cairo rules).
#2925 this updates torii to support new less strict upgrades for primitives
Summary by CodeRabbit
New Features
Improvements