refactor(torii-core): upgrade model silent if not in namespace #2688
Conversation
WalkthroughOhayo, sensei! This pull request modifies the error handling logic in 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: 0
🧹 Outside diff range and nitpick comments (2)
crates/torii/core/src/processors/upgrade_model.rs (1)
59-67: Consider making the error handling more robust, sensei!The current string-based error matching could be fragile if database error messages change. Consider creating a more robust solution.
Here's a suggested approach:
- if e.to_string().contains("no rows") { + // Add this helper at module level + fn is_not_found_error(e: &Error) -> bool { + // Consider using error downcast or specific error types + // depending on your database layer + e.to_string().contains("no rows") + } + + // In the match arm + if is_not_found_error(&e) {crates/torii/core/src/processors/upgrade_event.rs (1)
62-69: Consider extracting this error handling pattern, sensei! 🎋Since this error handling pattern might be reused across processors, consider extracting it into a helper function. Also, string matching for error types can be fragile.
Here's a suggested approach:
// In a common utility module pub(crate) fn handle_model_not_found<T>(result: Result<T, Error>) -> Result<Option<T>, Error> { match result { Ok(value) => Ok(Some(value)), Err(e) if e.to_string().contains("no rows") => Ok(None), Err(e) => Err(e), } } // In your processor let model = match handle_model_not_found(db.model(event.selector).await)? { Some(m) => m, None => return Ok(()), };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
crates/torii/core/src/processors/upgrade_event.rs(2 hunks)crates/torii/core/src/processors/upgrade_model.rs(2 hunks)
🔇 Additional comments (3)
crates/torii/core/src/processors/upgrade_model.rs (2)
1-1: Ohayo! Import cleanup looks good, sensei!
The removal of the explicit Ok import from anyhow is a nice cleanup since it's available via the prelude.
57-67: Ohayo! The error handling approach is solid, sensei!
The new match-based error handling elegantly handles the case where models don't exist in the indexed namespace, with clear comments explaining the behavior.
crates/torii/core/src/processors/upgrade_event.rs (1)
59-70: Ohayo! Clean and well-documented error handling implementation, sensei! 🎌
The new error handling approach elegantly handles the case where models don't exist in the namespace, which aligns perfectly with the PR objective. The added comments clearly explain the rationale.
Let's verify the consistency of this pattern across other processors:
✅ Verification successful
Ohayo! The error handling pattern is perfectly consistent across processors, sensei! ✨
The verification shows that the same "no rows" error handling pattern is consistently implemented across all relevant processors:
- store_del_record.rs
- store_update_member.rs
- store_set_record.rs
- store_update_record.rs
- upgrade_model.rs
- upgrade_event.rs (the file under review)
All processors follow the same pattern of silently returning Ok(()) when a model is not found, which aligns perfectly with the PR objective of handling models not in the namespace.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if similar error handling patterns exist in other processors
# Expected: Find similar patterns in upgrade_model.rs and potentially other processors
# Search for similar error handling patterns in processor files
rg -U "match.*model.*\{.*contains.*no rows.*Ok\(\(\)\).*\}" crates/torii/core/src/processors/
# Look for any other direct model queries that might need similar handling
ast-grep --pattern 'db.model($$$).await?'
Length of output: 139
Script:
#!/bin/bash
# Let's try a broader search for model-related error handling patterns
rg -U "match.*db\.model.*\{.*Err.*\}" crates/torii/core/src/processors/
# Search for any direct model queries in processor files
rg "db\.model\(" crates/torii/core/src/processors/
# Look for similar error handling patterns with 'no rows'
rg -U "no rows" crates/torii/core/src/processors/
# Search for all processor files to understand the context better
fd ".*processor\.rs$" crates/torii/core/src/processors/
Length of output: 1659
Script:
#!/bin/bash
# Let's check the actual error handling patterns in these files
rg -U -A 5 "no rows" crates/torii/core/src/processors/store_update_record.rs crates/torii/core/src/processors/store_update_member.rs crates/torii/core/src/processors/store_set_record.rs crates/torii/core/src/processors/store_del_record.rs
# Also check upgrade_model.rs specifically since it's closely related
cat crates/torii/core/src/processors/upgrade_model.rs
Length of output: 5943
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2688 +/- ##
==========================================
- Coverage 57.26% 57.24% -0.03%
==========================================
Files 408 408
Lines 51754 51791 +37
==========================================
+ Hits 29638 29648 +10
- Misses 22116 22143 +27 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
crates/torii/core/src/processors/store_del_record.rs (1)
58-66: Ohayo sensei! Consider using a custom error type for more idiomatic error handlingThe error handling and debug logging implementation looks good! However, checking error messages via string matching could be fragile if the underlying error message changes.
Consider defining a custom error type or using the database-specific error type for more robust error matching:
- Err(e) if e.to_string().contains("no rows") => { + Err(e) if matches!(e.downcast_ref(), Some(SqlError::NoRows)) => {This would require:
- Defining a custom error enum or using your SQL library's error type
- Using
downcast_ref()for type-safe error matchingThe current implementation is functional, but this change would make it more maintainable and type-safe.
crates/torii/core/src/processors/store_set_record.rs (1)
59-67: LGTM! Improved error handling with better observability.Ohayo sensei! The enhanced error handling is a solid improvement:
- Gracefully handles the "no rows" case with debug logging
- Maintains existing behavior while improving observability
- Follows Rust's idiomatic error handling patterns
Consider standardizing this pattern across other processors in the codebase for consistent error handling and logging behavior.
crates/torii/core/src/processors/store_update_record.rs (1)
Line range hint
1-100: Solid implementation approach overall!The changes demonstrate good practices:
- Error handling is explicit and maintainable
- Logging levels are appropriately chosen (debug for diagnostic info, info for business events)
- The code maintains clean Rust idioms
Consider adding metrics/counters for skipped models to help monitor the system's behavior in production.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
crates/torii/core/src/processors/store_del_record.rs(2 hunks)crates/torii/core/src/processors/store_set_record.rs(2 hunks)crates/torii/core/src/processors/store_update_record.rs(2 hunks)crates/torii/core/src/processors/upgrade_event.rs(2 hunks)crates/torii/core/src/processors/upgrade_model.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/core/src/processors/upgrade_event.rs
- crates/torii/core/src/processors/upgrade_model.rs
🔇 Additional comments (4)
crates/torii/core/src/processors/store_del_record.rs (1)
7-7: Ohayo! Clean import addition for logging capabilities!
The import of both debug and info levels is well-aligned with their usage in the code.
crates/torii/core/src/processors/store_set_record.rs (1)
7-7: LGTM! Clean import addition.
Ohayo! The addition of debug from tracing is well-aligned with the new error handling improvements.
crates/torii/core/src/processors/store_update_record.rs (2)
62-70: Ohayo! Clean error handling pattern, sensei!
The match pattern for database errors is well-implemented:
- Gracefully handles the "no rows" case with appropriate debug logging
- Properly propagates other database errors
- Maintains good observability with contextual logging
8-8: Good logging practices!
The debug-level logging for skipped models is appropriate and includes relevant context (selector) for troubleshooting.
Also applies to: 63-67
Summary by CodeRabbit