feat(torii-core): optimistically broadcast entity update #2466
Conversation
WalkthroughOhayo, sensei! This pull request introduces the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🔇 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: 3
Outside diff range and nitpick comments (2)
crates/torii/core/src/types.rs (1)
32-44: Ohayo, sensei! Consider adding documentation to clarify the relationship with Entity struct.The new
OptimisticEntityUpdatestruct shares many similarities with the existingEntitystruct. To improve code clarity and maintainability, it would be beneficial to add documentation explaining the relationship between these two structs and their specific use cases.Consider adding a doc comment above the struct definition, like this:
/// Represents an optimistic update to an entity. /// This struct is used for [explain specific use case], while the `Entity` struct is used for [explain Entity use case]. #[derive(FromRow, Deserialize, Debug, Clone)] #[serde(rename_all = "camelCase")] pub struct OptimisticEntityUpdate { // ... fields ... }crates/torii/grpc/src/server/subscriptions/entity.rs (1)
86-89: Ohayo, sensei! LGTM with a small suggestion.The
Servicestruct initialization now correctly usesOptimisticEntityUpdate, which aligns with the new optimistic concurrency model. Great job!Consider adjusting the indentation for better readability:
Self { subs_manager, - simple_broker: Box::pin(SimpleBroker::<OptimisticEntityUpdate>::subscribe()), + simple_broker: Box::pin( + SimpleBroker::<OptimisticEntityUpdate>::subscribe() + ), }
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- crates/torii/core/src/sql.rs (3 hunks)
- crates/torii/core/src/types.rs (1 hunks)
- crates/torii/grpc/src/server/subscriptions/entity.rs (2 hunks)
Additional comments not posted (6)
crates/torii/core/src/types.rs (2)
32-44: Ohayo, sensei! The new struct looks good overall.The
OptimisticEntityUpdatestruct is well-defined and consistent with the existing coding style. Nice work!
42-43: Ohayo! Can you clarify the implications of skipping the 'deleted' field in SQLx?The
deletedfield is marked with#[sqlx(skip)]. This means it won't be included in database operations by default.Could you explain how this field is intended to be used and updated? If it's meant to be handled separately from regular CRUD operations, a comment explaining this would be helpful.
To verify the usage of this field, let's run the following script:
Verification successful
Ohayo, sensei! 🌸 It looks like we need to explore a bit more to uncover how the
deletedfield is being used. Let's run a broader search to ensure we're not missing anything.
Ohayo, sensei! 🌸
Clarification on the
deletedField HandlingIt appears that the
deletedfield is managed within the application logic rather than through direct SQLx operations. This aligns with the usage patterns observed in the codebase.
- The field is utilized in various parts of the application, such as:
crates/torii/graphql/src/tests/models_test.rsfor testing deleted records.crates/torii/grpc/src/server/subscriptions/entity.rsto check deletion status.crates/torii/core/src/query_queue.rsfor managing entity updates and deletions.Recommendation:
- Consider adding a comment in
crates/torii/core/src/types.rsto explain that thedeletedfield is handled separately from regular CRUD operations. This will aid in understanding its purpose and usage within the application.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any custom handling of the 'deleted' field in SQL queries or database operations # Search for SQL queries or database operations involving 'deleted' rg --type rust -i 'deleted.*sql' crates/toriiLength of output: 47
Script:
#!/bin/bash # Description: Search for any usage of the 'deleted' field across all Rust files in crates/torii # Perform a case-insensitive search for 'deleted' in Rust files rg --type rust -i 'deleted' crates/toriiLength of output: 1004
crates/torii/grpc/src/server/subscriptions/entity.rs (2)
17-17: Ohayo, sensei! LGTM: New import for OptimisticEntityUpdate.The import statement for
OptimisticEntityUpdateis correctly added and aligns with the existing import style.
Line range hint
1-238: Ohayo, sensei! Overall great implementation of OptimisticEntityUpdate.The changes in this file consistently implement the transition to
OptimisticEntityUpdate, which should improve the concurrency model. The modifications are well-structured and maintain the existing code style. Great work on this update!crates/torii/core/src/sql.rs (2)
20-23: Ohayo, sensei! Appropriate Import ofSimpleBrokerandOptimisticEntityUpdateThe addition of
SimpleBrokerandOptimisticEntityUpdateis necessary for implementing the optimistic concurrency model.
244-253: Ohayo, sensei! Correct Integration ofOptimisticEntityUpdateinset_entityThe implementation of
OptimisticEntityUpdateand publishing it viaSimpleBrokerin theset_entitymethod enhances the entity update mechanism effectively.
crates/torii/core/src/sql.rs
Outdated
|
|
||
| let optimistic_entity_update = OptimisticEntityUpdate { | ||
| id: entity_id.clone(), | ||
| keys: keys_str.unwrap_or("").to_string(), | ||
| event_id: event_id.to_string(), | ||
| updated_model: Some(entity), | ||
| deleted: false, | ||
| }; | ||
| match optimistic_entity_update.updated_model { | ||
| Some(Ty::Struct(ref mut struct_)) => { | ||
| struct_.children.clear(); | ||
| } | ||
| _ => {} | ||
| } | ||
|
|
||
| SimpleBroker::publish(optimistic_entity_update); | ||
|
|
There was a problem hiding this comment.
Ohayo, sensei! Fix Undefined keys_str and Correct the deleted Flag in delete_entity
There are issues in this code segment:
- Undefined Variable:
keys_stris used but not defined in this scope, which will cause a compile-time error. - Incorrect
deletedFlag: Thedeletedflag is set tofalseduring an entity deletion. It should be set totrueto accurately represent the entity's deletion status.
To resolve these issues, consider making the following changes:
- Pass
keys_stras an argument to thedelete_entityfunction. - Set the
deletedflag totrueto indicate the entity has been deleted.
Apply this diff:
pub async fn delete_entity(
&mut self,
entity_id: Felt,
model_id: Felt,
entity: Ty,
+ keys_str: Option<&str>,
event_id: &str,
block_timestamp: u64,
) -> Result<()> {
let entity_id = format!("{:#x}", entity_id);
let path = vec![entity.name()];
let optimistic_entity_update = OptimisticEntityUpdate {
id: entity_id.clone(),
- keys: keys_str.unwrap_or("").to_string(),
+ keys: keys_str.unwrap_or("").to_string(),
event_id: event_id.to_string(),
updated_model: Some(entity),
- deleted: false,
+ deleted: true,
};
match optimistic_entity_update.updated_model {
Some(Ty::Struct(ref mut struct_)) => {
struct_.children.clear();
}
_ => {}
}
SimpleBroker::publish(optimistic_entity_update);Committable suggestion was skipped due to low confidence.
crates/torii/core/src/types.rs
Outdated
| pub keys: String, | ||
| pub event_id: String, | ||
|
|
||
| // this should never be None |
There was a problem hiding this comment.
Why using an option is it should never be none?
There was a problem hiding this comment.
Because we have a "FromRow" from sqlx. The row doesn't have that field so we put an option to then inject that fields value
Larkooo
force-pushed
the
optimisti-entity-update
branch
from
03b3307
to
5550d4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
crates/torii/grpc/src/server/subscriptions/entity.rs (1)
Line range hint
151-158: Potential panic due to use ofunwrap()onOptionUsing
unwrap()onOptiontypes can cause a panic if the value isNone. Even though the comment mentions thatupdated_modelshould never beNone, it's safer to handle this explicitly. Consider usingexpect()with a descriptive message or gracefully handling theNonecase to prevent potential runtime panics.Apply this diff to improve error handling:
- let model = entity.updated_model.as_ref().unwrap().as_struct().unwrap().clone(); + let model = entity + .updated_model + .as_ref() + .expect("updated_model should not be None") + .as_struct() + .expect("Failed to convert updated_model to struct") + .clone();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- crates/torii/core/src/executor.rs (4 hunks)
- crates/torii/core/src/types.rs (2 hunks)
- crates/torii/grpc/src/server/subscriptions/entity.rs (2 hunks)
- crates/torii/grpc/src/server/subscriptions/event_message.rs (2 hunks)
🔇 Additional comments (7)
crates/torii/grpc/src/server/subscriptions/event_message.rs (4)
17-17: Import ofOptimisticEventMessageis correctOhayo, sensei! The addition of
use torii_core::types::OptimisticEventMessage;is appropriate for the updated implementation.
75-75: Updatesimple_brokerto streamOptimisticEventMessageOhayo, sensei! The
simple_brokernow correctly streamsOptimisticEventMessage. Ensure that any logic consuming this stream handles the new message type appropriately.
80-83: Initializesimple_brokerwithOptimisticEventMessageOhayo, sensei! Confirm that
SimpleBroker::<OptimisticEventMessage>::subscribe()properly returns a stream ofOptimisticEventMessageinstances, and that this aligns with the expected behavior of your service.
88-88: Verify compatibility ofOptimisticEventMessagefields inpublish_updatesOhayo, sensei! Please ensure that
OptimisticEventMessageprovides the fields used withinpublish_updates, such asid,keys, andupdated_model, and that their data types and usages remain consistent with the previousEventMessage.Run the following script to confirm the presence and structure of these fields:
crates/torii/grpc/src/server/subscriptions/entity.rs (2)
81-89: Ohayo, sensei! Updatedsimple_brokerto useOptimisticEntitycorrectlyThe changes to
simple_brokerin theService::newmethod now utilizeOptimisticEntity, which is consistent with the shift towards the optimistic concurrency model. This ensures that the service broadcasts updates using the new entity type.
92-95: Ohayo, sensei! Updatedpublish_updatesto acceptOptimisticEntityThe
publish_updatesfunction now accepts a reference toOptimisticEntity, aligning with the updated data model across the codebase. This change ensures that entity updates are handled optimistically as intended.crates/torii/core/src/executor.rs (1)
18-18: Ohayo, sensei! Importing new types for optimistic handlingThe addition of
OptimisticEntityandOptimisticEventMessageto the imports aligns with their usage in the code.
| #[derive(FromRow, Deserialize, Debug, Clone)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct OptimisticEntity { | ||
| pub id: String, | ||
| pub keys: String, | ||
| pub event_id: String, | ||
| pub executed_at: DateTime<Utc>, | ||
| pub created_at: DateTime<Utc>, | ||
| pub updated_at: DateTime<Utc>, | ||
|
|
||
| // this should never be None | ||
| #[sqlx(skip)] | ||
| pub updated_model: Option<Ty>, | ||
| #[sqlx(skip)] | ||
| pub deleted: bool, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Consider refactoring to avoid duplication.
The OptimisticEntity structure appears to be identical to the existing Entity structure. To maintain DRY (Don't Repeat Yourself) principles, consider using a type alias instead:
pub type OptimisticEntity = Entity;This approach would reduce code duplication and make maintenance easier. If there are specific reasons for keeping them separate, please add a comment explaining the rationale.
Also, the updated_model field is still using Option<Ty>. If it should never be None as per the comment, consider changing it to:
pub updated_model: Ty,If Option is necessary due to SQLx requirements, please add a comment explaining this constraint.
| #[derive(FromRow, Deserialize, Debug, Clone)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct OptimisticEventMessage { | ||
| pub id: String, | ||
| pub keys: String, | ||
| pub event_id: String, | ||
| pub executed_at: DateTime<Utc>, | ||
| pub created_at: DateTime<Utc>, | ||
| pub updated_at: DateTime<Utc>, | ||
|
|
||
| // this should never be None | ||
| #[sqlx(skip)] | ||
| pub updated_model: Option<Ty>, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo again, sensei! Let's discuss the OptimisticEventMessage structure.
The OptimisticEventMessage structure is nearly identical to the EventMessage structure, with one key difference: it's missing the deleted field. Here are some suggestions:
- If
OptimisticEventMessagedoesn't need thedeletedfield, consider using a type alias with a custom type:
pub struct OptimisticEventMessageData {
pub event_message: EventMessage,
// Add any additional fields specific to OptimisticEventMessage
}
pub type OptimisticEventMessage = OptimisticEventMessageData;- If the
deletedfield should be present, add it to maintain consistency:
#[sqlx(skip)]
pub deleted: bool,- If there's a specific reason for the current implementation, please add a comment explaining the rationale behind the differences between
EventMessageandOptimisticEventMessage.
These changes would improve code clarity and maintainability. What do you think, sensei?
crates/torii/core/src/executor.rs
Outdated
| let optimistic_entity = OptimisticEntity { | ||
| id: entity_updated.id.clone(), | ||
| keys: entity_updated.keys.clone(), | ||
| event_id: entity_updated.event_id.clone(), | ||
| executed_at: entity_updated.executed_at.clone(), | ||
| created_at: entity_updated.created_at.clone(), | ||
| updated_at: entity_updated.updated_at.clone(), | ||
| updated_model: entity_updated.updated_model.clone(), | ||
| deleted: entity_updated.deleted, | ||
| }; | ||
| SimpleBroker::publish(optimistic_entity); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce code duplication when creating OptimisticEntity
This block duplicates field cloning from entity_updated to create OptimisticEntity. To improve maintainability and reduce code duplication, consider implementing the From trait to convert EntityUpdated into OptimisticEntity.
Implement the From trait for OptimisticEntity:
impl From<&EntityUpdated> for OptimisticEntity {
fn from(entity_updated: &EntityUpdated) -> Self {
Self {
id: entity_updated.id.clone(),
keys: entity_updated.keys.clone(),
event_id: entity_updated.event_id.clone(),
executed_at: entity_updated.executed_at.clone(),
created_at: entity_updated.created_at.clone(),
updated_at: entity_updated.updated_at.clone(),
updated_model: entity_updated.updated_model.clone(),
deleted: entity_updated.deleted,
}
}
}Then, you can simplify the code to:
let optimistic_entity = OptimisticEntity::from(&entity_updated);
crates/torii/core/src/executor.rs
Outdated
| let optimistic_entity = OptimisticEntity { | ||
| id: entity_updated.id.clone(), | ||
| keys: entity_updated.keys.clone(), | ||
| event_id: entity_updated.event_id.clone(), | ||
| executed_at: entity_updated.executed_at.clone(), | ||
| created_at: entity_updated.created_at.clone(), | ||
| updated_at: entity_updated.updated_at.clone(), | ||
| updated_model: entity_updated.updated_model.clone(), | ||
| deleted: entity_updated.deleted, | ||
| }; | ||
| SimpleBroker::publish(optimistic_entity); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce code duplication when creating OptimisticEntity
This block repeats the same cloning logic as before. Using the From trait implementation suggested earlier will reduce code duplication and improve maintainability.
crates/torii/core/src/executor.rs
Outdated
| let optimistic_event_message = OptimisticEventMessage { | ||
| id: event_message.id.clone(), | ||
| keys: event_message.keys.clone(), | ||
| event_id: event_message.event_id.clone(), | ||
| executed_at: event_message.executed_at.clone(), | ||
| created_at: event_message.created_at.clone(), | ||
| updated_at: event_message.updated_at.clone(), | ||
| updated_model: event_message.updated_model.clone(), | ||
| }; | ||
| SimpleBroker::publish(optimistic_event_message); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce code duplication when creating OptimisticEventMessage
This block duplicates field cloning from event_message to create OptimisticEventMessage. To improve maintainability and reduce code duplication, consider implementing the From trait to convert EventMessageUpdated into OptimisticEventMessage.
Implement the From trait for OptimisticEventMessage:
impl From<&EventMessageUpdated> for OptimisticEventMessage {
fn from(event_message: &EventMessageUpdated) -> Self {
Self {
id: event_message.id.clone(),
keys: event_message.keys.clone(),
event_id: event_message.event_id.clone(),
executed_at: event_message.executed_at.clone(),
created_at: event_message.created_at.clone(),
updated_at: event_message.updated_at.clone(),
updated_model: event_message.updated_model.clone(),
}
}
}Then, you can simplify the code to:
let optimistic_event_message = OptimisticEventMessage::from(&event_message);
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- crates/torii/core/src/executor.rs (4 hunks)
🔇 Additional comments (2)
crates/torii/core/src/executor.rs (2)
18-18: Ohayo, sensei! LGTM!The addition of
OptimisticEntityandOptimisticEventMessageto the imports is correct and necessary for their usage in the file.
Line range hint
1-338: Ohayo, sensei! Let's wrap up this review!Great job implementing optimistic updates for entities and event messages! The changes effectively introduce
OptimisticEntityandOptimisticEventMessageto support this feature. To further enhance your code, consider implementing theFromtrait for both types as suggested in the previous comments. This will reduce code duplication and improve maintainability.Overall, the changes look solid and will contribute to a more responsive and efficient system. Keep up the excellent work, sensei!
| let optimistic_entity = OptimisticEntity { | ||
| id: entity_updated.id.clone(), | ||
| keys: entity_updated.keys.clone(), | ||
| event_id: entity_updated.event_id.clone(), | ||
| executed_at: entity_updated.executed_at, | ||
| created_at: entity_updated.created_at, | ||
| updated_at: entity_updated.updated_at, | ||
| updated_model: entity_updated.updated_model.clone(), | ||
| deleted: entity_updated.deleted, | ||
| }; | ||
| SimpleBroker::publish(optimistic_entity); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Consider implementing From trait for OptimisticEntity
The creation of OptimisticEntity duplicates field assignments from entity_updated. To reduce code duplication and improve maintainability, consider implementing the From trait for OptimisticEntity as suggested in a previous review comment.
Example implementation:
impl From<&EntityUpdated> for OptimisticEntity {
fn from(entity: &EntityUpdated) -> Self {
Self {
id: entity.id.clone(),
keys: entity.keys.clone(),
event_id: entity.event_id.clone(),
executed_at: entity.executed_at,
created_at: entity.created_at,
updated_at: entity.updated_at,
updated_model: entity.updated_model.clone(),
deleted: entity.deleted,
}
}
}Then, you can simplify the code to:
let optimistic_entity = OptimisticEntity::from(&entity_updated);
SimpleBroker::publish(optimistic_entity);This change will make the code more concise and easier to maintain, sensei!
| let optimistic_entity = OptimisticEntity { | ||
| id: entity_updated.id.clone(), | ||
| keys: entity_updated.keys.clone(), | ||
| event_id: entity_updated.event_id.clone(), | ||
| executed_at: entity_updated.executed_at, | ||
| created_at: entity_updated.created_at, | ||
| updated_at: entity_updated.updated_at, | ||
| updated_model: entity_updated.updated_model.clone(), | ||
| deleted: entity_updated.deleted, | ||
| }; | ||
| SimpleBroker::publish(optimistic_entity); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo again, sensei! Let's use that From trait here too!
This block is almost identical to the one in the QueryType::SetEntity branch. Once you've implemented the From trait for OptimisticEntity as suggested earlier, you can simplify this code in the same way:
let optimistic_entity = OptimisticEntity::from(&entity_updated);
SimpleBroker::publish(optimistic_entity);This change will maintain consistency across the codebase and reduce duplication, making it easier to maintain and understand. Keep up the great work, sensei!
| let optimistic_event_message = OptimisticEventMessage { | ||
| id: event_message.id.clone(), | ||
| keys: event_message.keys.clone(), | ||
| event_id: event_message.event_id.clone(), | ||
| executed_at: event_message.executed_at, | ||
| created_at: event_message.created_at, | ||
| updated_at: event_message.updated_at, | ||
| updated_model: event_message.updated_model.clone(), | ||
| }; | ||
| SimpleBroker::publish(optimistic_event_message); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo once more, sensei! Let's complete the trifecta with From for OptimisticEventMessage!
To maintain consistency and reduce code duplication, let's apply the same pattern we used for OptimisticEntity to OptimisticEventMessage. Consider implementing the From trait for OptimisticEventMessage:
impl From<&EventMessageUpdated> for OptimisticEventMessage {
fn from(event: &EventMessageUpdated) -> Self {
Self {
id: event.id.clone(),
keys: event.keys.clone(),
event_id: event.event_id.clone(),
executed_at: event.executed_at,
created_at: event.created_at,
updated_at: event.updated_at,
updated_model: event.updated_model.clone(),
}
}
}Then, simplify the code to:
let optimistic_event_message = OptimisticEventMessage::from(&event_message);
SimpleBroker::publish(optimistic_event_message);This change will complete the refactoring trifecta, making your code more consistent, maintainable, and elegant. You're doing great work, sensei!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2466 +/- ##
=======================================
Coverage ? 68.82%
=======================================
Files ? 377
Lines ? 48930
Branches ? 0
=======================================
Hits ? 33677
Misses ? 15253
Partials ? 0 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
OptimisticEntityUpdate,OptimisticEntity, andOptimisticEventMessage, enhancing the entity and event message handling.OptimisticEntityandOptimisticEventMessage, improving message publishing and subscription management.Bug Fixes
Documentation