feat(torii): historical entity updates #3090
Conversation
WalkthroughOhayo sensei! This pull request introduces a new SQL migration that creates an Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (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: 0
🧹 Nitpick comments (1)
crates/torii/migrations/20250310063604_historical_entities.sql (1)
1-2: Ohayo sensei, clear and welcoming comments!
The header comments nicely explain the purpose of the migration and the design rationale. Just a nitpick: the term "historicallity" may be a bit unconventional—if it's intentional domain slang, that's great; otherwise, consider a more standard term like "history tracking" for clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/migrations/20250310063604_historical_entities.sql(1 hunks)
🔇 Additional comments (2)
crates/torii/migrations/20250310063604_historical_entities.sql (2)
3-16: Ohayo sensei, solid table creation logic!
Theentities_historicaltable is defined with the expected columns and default timestamp settings. The decision to omit a primary key is clearly documented and appears to support the one-to-many relationship design. However, please verify that this design aligns with your application’s data integrity requirements—if uniqueness for each row might be needed later, you might consider a composite or surrogate key.
18-20: Ohayo sensei, the ALTER TABLE statement looks good!
Adding thehistorical_countercolumn to theentity_modeltable with a default value of 0 is well executed. Ensure that theentity_modeltable exists and that this schema change integrates smoothly with existing application logic.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (7)
crates/torii/cli/src/options.rs(1 hunks)crates/torii/indexer/src/processors/mod.rs(1 hunks)crates/torii/indexer/src/processors/store_set_record.rs(2 hunks)crates/torii/indexer/src/processors/store_update_member.rs(2 hunks)crates/torii/indexer/src/processors/store_update_record.rs(2 hunks)crates/torii/sqlite/src/executor/mod.rs(2 hunks)crates/torii/sqlite/src/lib.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: docs
- GitHub Check: ensure-wasm
- GitHub Check: clippy
🔇 Additional comments (14)
crates/torii/indexer/src/processors/store_set_record.rs (2)
5-5: New import added for tag-based historical tracking.Ohayo sensei! You've added the
get_tagimport from dojo_types, which will be used to determine if a model should have historical data tracking enabled.
106-115: Historical tracking implementation added to entity processing.The implementation correctly determines if a model should be tracked historically by combining the namespace and name to create a tag, then checking against the config. This is a clean approach that maintains separation of concerns and adds the new functionality without disrupting the existing flow.
crates/torii/indexer/src/processors/mod.rs (2)
36-36: Terminology change from 'historical_events' to 'historical_models'.Good semantic change sensei! This properly reflects that historical tracking is applied at the model level rather than the event level, which aligns better with the overall architecture.
43-43: Updated is_historical method to use the renamed field.The method implementation has been correctly updated to match the field rename, maintaining consistency throughout the codebase.
crates/torii/indexer/src/processors/store_update_member.rs (2)
5-5: New import added for tag-based historical tracking.Ohayo! This import addition mirrors the same change in store_set_record.rs, maintaining consistency across processor implementations.
125-135: Historical tracking implementation added to entity processing.The implementation properly determines if a model should be tracked historically and passes this information to the database layer. The whitespace restructuring also improves readability.
crates/torii/sqlite/src/lib.rs (1)
322-322: Added is_historical parameter to set_entity method.This parameter addition allows the caller to specify whether the entity should be tracked historically, which is essential for the feature being implemented.
crates/torii/cli/src/options.rs (1)
276-277: Documentation update looks good!Ohayo sensei! The documentation update for the
historicalfield is clearer and more comprehensive now, indicating that historical tracking applies to both event messages and entities.Also applies to: 281-281
crates/torii/indexer/src/processors/store_update_record.rs (2)
5-5: New import added for tag generation.Ohayo sensei! Added import of
get_tagfunction fromdojo_types::namingto support historical entity functionality.
117-126: Historical entity handling implemented correctly.Nicely done! The implementation correctly determines whether the entity should be treated as historical based on the model tag, and passes this information to the database layer. This aligns well with the PR objective to add historical entity tracking.
crates/torii/sqlite/src/executor/mod.rs (4)
107-116: Good struct addition for entity queries!Ohayo sensei! This new
EntityQuerystruct is well-designed and mirrors the existingEventMessageQuerystructure, maintaining consistency in the codebase. The addition of theis_historicalfield enables the tracking of historical entities.
123-123: QueryType enum updated appropriately.The
SetEntityvariant now uses the newEntityQuerytype instead of justTy, which provides the necessary context for historical tracking.
462-462: Model reference fixed.Updated to use
entity.ty.clone()which is more appropriate than using the entire entity.
470-508: Historical entity tracking logic well implemented!Excellent implementation of historical entity tracking! The code follows the same pattern used for historical event messages, maintaining consistency throughout the codebase. The logic correctly:
- Queries the current historical counter
- Increments it for historical entities
- Stores historical data in the
entities_historicaltable- Updates the counter in the
entity_modeltableThis approach provides a good foundation for querying historical entity states.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/sqlite/src/executor/mod.rs (1)
107-522: Consider adding documentation for the historical entity featureOhayo sensei! While the implementation is solid, it would be helpful to add some comments explaining the purpose of the historical entity tracking system and how
historical_counteris used. This would make it easier for future developers to understand the design intent.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
crates/torii/migrations/20250310063604_historical_entities.sql(1 hunks)crates/torii/sqlite/src/executor/mod.rs(2 hunks)crates/torii/sqlite/src/lib.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/migrations/20250310063604_historical_entities.sql
- crates/torii/sqlite/src/lib.rs
🔇 Additional comments (6)
crates/torii/sqlite/src/executor/mod.rs (6)
107-116: Well-structured struct for entity queries!Ohayo sensei! The new
EntityQuerystruct looks nicely organized with all the fields needed for entity operations, including the newis_historicalflag. This struct will make tracking historical entity changes much cleaner.
123-123: Good refactoring of the QueryType enum!The change from
SetEntity(Ty)toSetEntity(EntityQuery)is a solid improvement, sensei! It encapsulates all entity-related information in a single structure, making the code more maintainable and the interface more consistent.
462-462: Using entity.ty.clone() instead of entity is more precise!Nice optimization, sensei! You're now just using the type information rather than the entire entity object.
469-479: Historical entity counter retrieval looks good!The implementation matches the pattern used for historical event messages. The query handles the case when no record exists by defaulting to 0, which is a good defensive approach.
480-510: Solid implementation of historical entity tracking!The conditional logic for handling historical entities is well-structured. It:
- Increments the counter for historical entities
- Serializes entity data to JSON
- Properly handles both cases - with and without keys
- Uses parameterized queries to prevent SQL injection
Good attention to detail, sensei!
512-522: Proper update of entity_model with historical counter!The upsert query is well-crafted with the
ON CONFLICTclause to handle both insert and update scenarios. This ensures the historical counter is always properly maintained.
#3030
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Chores