fix(torii-sqlite): deleting entity #2961
Conversation
WalkthroughOhayo, sensei! The pull request modifies the Changes
Suggested Reviewers
Possibly Related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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] 449-453: Code formatting error: Vector arguments need to be formatted across multiple lines according to rustfmt style
🔇 Additional comments (1)
crates/torii/sqlite/src/lib.rs (1)
Line range hint
449-458: Implementation looks good!The query correctly handles entity deletion by:
- Removing the entity from its model-specific table
- Cleaning up the entity-model mapping
- Using parameterized queries for security
🧰 Tools
🪛 GitHub Actions: ci
[error] 449-453: Code formatting error: Vector arguments need to be formatted across multiple lines according to rustfmt style
crates/torii/sqlite/src/lib.rs
Outdated
| @@ -449,7 +449,7 @@ impl Sql { | |||
| WHERE entity_id = ? AND model_id = ?" | |||
| ) | |||
| .to_string(), | |||
| vec![Argument::String(entity_id.clone()), Argument::String(format!("{:#x}", model_id))], | |||
| vec![Argument::String(entity_id.clone()), Argument::String(entity_id.clone()), Argument::String(format!("{:#x}", model_id))], | |||
There was a problem hiding this comment.
Ohayo sensei! Fix the formatting issue.
The vector arguments need to be formatted across multiple lines according to rustfmt style.
Apply this diff to fix the formatting:
- vec![Argument::String(entity_id.clone()), Argument::String(entity_id.clone()), Argument::String(format!("{:#x}", model_id))],
+ vec![
+ Argument::String(entity_id.clone()),
+ Argument::String(entity_id.clone()),
+ Argument::String(format!("{:#x}", model_id)),
+ ],📝 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.
| vec![Argument::String(entity_id.clone()), Argument::String(entity_id.clone()), Argument::String(format!("{:#x}", model_id))], | |
| vec![ | |
| Argument::String(entity_id.clone()), | |
| Argument::String(entity_id.clone()), | |
| Argument::String(format!("{:#x}", model_id)), | |
| ], |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/sqlite/src/executor/mod.rs (1)
478-482: Consider adding transaction rollback handling.Ohayo, sensei! While the deletion logic is correct, we should ensure atomicity between the
entity_modelandentitiestable operations. If the subsequent operations fail, we might want to rollback this deletion.Consider wrapping the operations in a transaction block or ensuring the existing transaction handling covers this case:
+ // Ensure this operation is part of the transaction sqlx::query("DELETE FROM entity_model WHERE entity_id = ? AND model_id = ?") .bind(entity.entity_id.clone()) .bind(entity.model_id) .execute(&mut **tx) .await?;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/torii/sqlite/src/executor/mod.rs(2 hunks)crates/torii/sqlite/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/sqlite/src/lib.rs
🔇 Additional comments (2)
crates/torii/sqlite/src/executor/mod.rs (2)
56-56: LGTM! Addition of model_id field.The addition of the
model_idfield toDeleteEntityQueryis well-structured and aligns with the entity-model relationship requirements.
Line range hint
478-524: Verify cascading delete behavior.The deletion logic appears to handle the entity-model relationship correctly, but we should verify the behavior when:
- Multiple models reference the same entity
- The last model for an entity is deleted
✅ Verification successful
Ohayo sensei! The cascading delete implementation is correct and safe
The code properly handles both scenarios:
- Multiple models can reference the same entity (ensured by the schema's unique constraint)
- Entity deletion occurs only when its last model is removed (verified by the count check)
The implementation is protected by proper foreign key constraints and transaction boundaries, preventing any inconsistent states.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential orphaned entities or inconsistent states ast-grep --pattern 'DELETE FROM entities WHERE id = $_' rg -A 5 'DELETE FROM entity_model'Length of output: 592
Script:
#!/bin/bash # Check schema and other SQL operations echo "=== Schema ===" rg -A 10 'CREATE TABLE entities' rg -A 10 'CREATE TABLE entity_model' echo "=== Transaction handling ===" ast-grep --pattern 'transaction|BEGIN|COMMIT|ROLLBACK' echo "=== All operations on these tables ===" rg -B 2 -A 2 'FROM entities|FROM entity_model'Length of output: 6729
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
crates/torii/indexer/src/test.rs (1)
Ohayo sensei! The test flakiness is caused by a race condition between the executor task and database verification.
The issue occurs because:
- The executor task runs independently via
tokio::spawn- Database verification happens immediately after transaction completion
- No synchronization exists between event processing and database state
To fix:
- Add explicit synchronization between the executor and database verification
- Consider using a channel to signal when event processing is complete
- Add retry mechanism for database state verification
🔗 Analysis chain
Line range hint
1-1: Investigate and fix the flaky test behavior.The test is marked as flaky with an
#[ignore]attribute. Let's verify the potential race conditions or timing issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze the test execution patterns to identify flaky behavior. # Search for async operations and potential race conditions rg -A 5 "async|await|spawn|send|execute" crates/torii/indexer/src/test.rs # Look for timing-related code rg "sleep|timeout|delay" crates/torii/indexer/src/test.rs # Check for similar patterns in related files rg "test_load_from_remote" crates/torii/Length of output: 14439
Script:
#!/bin/bash # Compare the structure and patterns between the working and flaky tests # Check for error handling patterns rg "unwrap|expect|Error" crates/torii/indexer/src/test.rs # Look for transaction retry or validation mechanisms rg "retry|validate|confirm" crates/torii/indexer/src/ # Check the Executor implementation for race condition handling ast-grep --pattern 'struct Executor { $$$ }' # Look for similar test patterns in other files rg "katana_runner::test.*async fn test" crates/torii/Length of output: 7958
Script:
#!/bin/bash # Examine Executor and event processing implementation # Check Executor implementation rg -A 10 "struct Executor" crates/torii/indexer/src/ # Look for event processing logic rg -A 5 "process_range|process_event" crates/torii/indexer/src/ # Check for any synchronization mechanisms rg "Mutex|RwLock|channel|sync" crates/torii/indexer/src/ # Look for transaction processing logic rg -A 5 "TransactionWaiter" crates/torii/Length of output: 19887
🧹 Nitpick comments (1)
crates/torii/indexer/src/test.rs (1)
308-309: Let's address the TODO comment about chronological testing.The TODO comment suggests improving test chronology with Torii re-syncing. Consider implementing a test helper that allows controlling the sync process step by step.
Would you like me to help create a test helper that provides better control over the Torii syncing process?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/indexer/src/test.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: docs
- GitHub Check: ensure-wasm
- GitHub Check: clippy
🔇 Additional comments (1)
crates/torii/indexer/src/test.rs (1)
303-306: Ohayo! The assertions look good, sensei!The updated assertions correctly verify the entity deletion behavior by checking both the entity model relation and the entity itself.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2961 +/- ##
==========================================
+ Coverage 56.71% 57.19% +0.48%
==========================================
Files 420 423 +3
Lines 55557 56125 +568
==========================================
+ Hits 31507 32100 +593
+ Misses 24050 24025 -25 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit