feat(torii-sqlite): primitive upgrades towards bigger types #2931
Conversation
|
Ohayo sensei, here’s the detailed summary for the changes: WalkthroughThis PR updates the schema upgrade process in two modules. In the SQLite module, the Changes
Sequence Diagram(s)SQLite Column Upgrade ProcesssequenceDiagram
participant CM as Column Manager
participant DB as Database
participant TT as Temporary Table
CM->>CM: Validate type upgrade
alt Valid Upgrade
CM->>DB: Create temporary table
DB-->>CM: Temporary table created
CM->>DB: Copy data to temporary table
CM->>DB: Drop old column
CM->>CM: Perform SQL value conversion
CM->>DB: Modify column with new SQL type
else Invalid Upgrade
CM-->>CM: Return error indicating invalid upgrade
end
Schema Diff Calculation ProcesssequenceDiagram
participant P as Processor
participant Prev as Previous Schema
participant New as New Schema
P->>Prev: Call diff(New)
Prev-->>P: Return schema differences
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (2)
🪧 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.
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?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/dojo/types/src/primitive.rs (1)
406-443: Ohayo sensei! Consider adding dedicated tests forcan_upgrade_to.
Ensuring thorough coverage of each enumerated upgrade path will help catch future regressions for these conversions.crates/torii/sqlite/src/lib.rs (1)
1036-1038: Ohayo sensei! The pipeline complains about long comment lines.
Splitting them into shorter segments (typically 80-120 characters) might help satisfy lint rules.🧰 Tools
🪛 GitHub Actions: ci
[warning] 1036-1038: Comment line exceeds the recommended length. Please split the comment into multiple lines.
📜 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)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/sqlite/src/lib.rs
[warning] 1036-1038: Comment line exceeds the recommended length. Please split the comment into multiple lines.
🔇 Additional comments (2)
crates/torii/sqlite/src/lib.rs (2)
8-8: Ohayo sensei! Looks good.
ImportingSqlTypehere is straightforward and helps clarify usage.
1036-1066: Ohayo sensei! Please revisit transaction safety and parameterization.
This column-modification pattern is still prone to partial updates if a step fails, and string formatting for SQL can be risky.🧰 Tools
🪛 GitHub Actions: ci
[warning] 1036-1038: Comment line exceeds the recommended length. Please split the comment into multiple lines.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2931 +/- ##
==========================================
+ Coverage 56.97% 57.15% +0.17%
==========================================
Files 426 427 +1
Lines 56376 56754 +378
==========================================
+ Hits 32119 32435 +316
- Misses 24257 24319 +62 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/torii/sqlite/src/lib.rs (1)
884-891:⚠️ Potential issueOhayo, sensei! Address potential SQL injection vulnerability in
modify_column.The
sql_valueis still directly interpolated into the SQL statement without parameterization:"UPDATE [{table_id}] SET [{name}] = (SELECT {sql_value} FROM tmp_values_{name} WHERE tmp_values_{name}.internal_id = [{table_id}].internal_id)"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.
🧰 Tools
🪛 GitHub Actions: ci
[warning] 888-888: Line exceeds recommended length. Consider breaking the line for better readability.
🧹 Nitpick comments (1)
crates/torii/sqlite/src/lib.rs (1)
1036-1067: Ohayo, sensei! The primitive type upgrade logic looks solid!The implementation:
- Properly validates type upgrades using
can_upgrade_to- Handles integer to hex string conversion for larger numeric types
- Returns descriptive errors for invalid upgrades
However, consider breaking long lines for better readability:
- let sql_value = if old_primitive.to_sql_type() == SqlType::Integer && p.to_sql_type() == SqlType::Text { + let sql_value = if old_primitive.to_sql_type() == SqlType::Integer + && p.to_sql_type() == SqlType::Text {🧰 Tools
🪛 GitHub Actions: ci
[warning] 1042-1042: Line exceeds recommended length. Consider breaking the line for better readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/sqlite/src/lib.rs(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/sqlite/src/lib.rs
[warning] 881-881: Line exceeds recommended length. Consider breaking the line for better readability.
[warning] 888-888: Line exceeds recommended length. Consider breaking the line for better readability.
[warning] 1042-1042: Line exceeds recommended length. Consider breaking the line for better readability.
🔇 Additional comments (1)
crates/torii/sqlite/src/lib.rs (1)
8-8: LGTM!The import of
SqlTypeis necessary for the new primitive type upgrade functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/torii/sqlite/src/lib.rs (1)
891-894:⚠️ Potential issueOhayo, sensei! Address potential SQL injection vulnerability.
The SQL value is directly interpolated into the query without parameterization, which could expose the code to SQL injection attacks.
🧹 Nitpick comments (1)
crates/torii/sqlite/src/lib.rs (1)
1039-1070: Ohayo, sensei! Robust primitive type upgrade validation.The implementation ensures data integrity during schema upgrades by:
- Validating upgrade compatibility using
can_upgrade_to- Handling integer to text conversions with proper hex formatting
- Failing early for invalid upgrades with clear error messages
However, consider adding transaction handling to ensure atomic operations:
Apply this diff to ensure atomic operations:
fn add_columns_recursive( path: &[String], ty: &Ty, columns: &mut Vec<String>, alter_table_queries: &mut Vec<String>, indices: &mut Vec<String>, table_id: &str, upgrade_diff: Option<&Ty>, ) -> Result<()> { + // Start transaction for atomic operations + alter_table_queries.push("BEGIN TRANSACTION;".to_string()); + // ... existing code ... + + // Commit transaction on success + alter_table_queries.push("COMMIT;".to_string()); + // Add rollback for error handling + alter_table_queries.push("ROLLBACK;".to_string()); Ok(()) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/torii/indexer/src/processors/upgrade_event.rs(1 hunks)crates/torii/indexer/src/processors/upgrade_model.rs(1 hunks)crates/torii/sqlite/src/lib.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: docs
- GitHub Check: clippy
- GitHub Check: ensure-wasm
🔇 Additional comments (2)
crates/torii/indexer/src/processors/upgrade_model.rs (1)
95-95: Ohayo, sensei! LGTM - Consistent schema diff calculation.The change aligns with the similar modification in
upgrade_event.rs, ensuring consistent schema difference calculations across processors.crates/torii/indexer/src/processors/upgrade_event.rs (1)
97-97: Ohayo, sensei! LGTM - Consistent schema diff calculation.The change aligns with the similar modification in
upgrade_model.rs, ensuring consistent schema difference calculations across processors.
|
Went ahead and removed the |
There was a problem hiding this comment.
Thanks @Larkooo, works well for regular primitive types. However when the primitive is a type for an enum variant, it fails with the same issue as before:
Apart from that, it looks good, and the issue of enum variant may be tackled in a different PR. If you have a chance to add test in this one could be great, or maybe in a subsequent one where the variant type could be fixed?
Ok should be fixed. Actually that issue occurs for any nested primitives (when their column name contains a ".") which makes the queries fail. |
#2925 this updates torii to support new less strict upgrades for primitives
Summary by CodeRabbit
Bug Fixes
Refactor