refactor/fix(torii-core): correctly queue entity deletions #2428
Conversation
WalkthroughOhayo, sensei! This pull request introduces a new struct, Changes
Possibly related PRs
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
Files selected for processing (2)
- crates/torii/core/src/query_queue.rs (2 hunks)
- crates/torii/core/src/sql.rs (2 hunks)
Additional comments not posted (5)
crates/torii/core/src/query_queue.rs (2)
43-43: Ohayo, sensei! The newDeleteEntityvariant looks good.Adding the
DeleteEntity(Ty)variant to theQueryTypeenum is a great way to enable the system to process entity deletion requests. This change aligns perfectly with the PR objectives.
101-124: Ohayo! TheDeleteEntityimplementation is on point, sensei.The new
matcharm for theDeleteEntityvariant handles the entity deletion flow effectively:
- It fetches the entity details and checks if all associated models are deleted by querying the count from the
entity_modeltable.- If the count is zero, indicating that all models are deleted, it proceeds to delete the entity from the
entitiestable.- Finally, it publishes a
BrokerMessage::EntityUpdatedto maintain the system's messaging integrity.This implementation ensures that entities are deleted only when all their associated models are removed, maintaining data consistency. Great work!
crates/torii/core/src/sql.rs (3)
328-335: Ohayo sensei! The entity model deletion query looks solid.The prepared statement correctly deletes rows matching the provided
entity_idand computedmodel_id. Nicely done!
738-742: Nice catch with the enum option tuple check!Wrapping the tuple emptiness check inside the
if letto first confirm thetyis aTuplevariant is a smart move. This will prevent any unexpected panics if the option type doesn't match the assumption.Good attention to detail sensei!
337-345: The entity update query looks good, but let's double-check one thing.The query correctly updates the entity's
updated_at,executed_atandevent_idvalues. Associating it withQueryType::DeleteEntityis a smart way to track entities queued for deletion.Just one thing to verify sensei - are there any other places in the codebase that rely on the
updated_attimestamp of deleted entities? If so, updating it here might lead to unexpected behavior. Worth a quick check!Here's a script to search for
updated_atusage onentities:Let me know if you find any concerning usages!
Verification successful
Ohayo, sensei! I've taken a deep dive into the codebase, and I've got some good news for you!
No concerning usages of
updated_atfor deleted entities were found.The search results show that
updated_atis only being used in UPDATE queries for theentitiestable, which is exactly what we'd expect. There are no SELECT queries or other operations that seem to rely on theupdated_attimestamp for deleted entities.The current implementation appears to be correct and safe. You can proceed with confidence, knowing that updating
updated_atfor deleted entities shouldn't lead to any unexpected behavior in other parts of the codebase.Keep up the great work, sensei!
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for SELECT queries on `entities` that reference `updated_at` rg --type rust 'SELECT(.*)updated_at(.*)FROM(.*)entities' -A 5 # Search for any other usage of `updated_at` on `entities` rg --type rust 'entities(.*)updated_at' -A 5Length of output: 1053
crates/torii/core/src/query_queue.rs
Outdated
| let count = sqlx::query_scalar::<_, i64>("SELECT count(*) FROM entity_model WHERE entity_id = ?") | ||
| .bind(entity_updated.id.clone()) | ||
| .fetch_one(&mut *tx) | ||
| .await?; | ||
| entity_updated.deleted = count == 0; |
There was a problem hiding this comment.
Suggestion: Optimize the query by using a LEFT JOIN.
Ohayo, sensei! While the current implementation of fetching the count of associated models works, it requires an additional database query. To optimize the performance, consider using a LEFT JOIN with the main query to fetch the entity details and the count of associated models in a single query.
Here's an example of how you can modify the query:
SELECT e.*, COUNT(em.entity_id) AS model_count
FROM entities e
LEFT JOIN entity_model em ON e.id = em.entity_id
WHERE e.id = ?
GROUP BY e.idThis query will return the entity details along with the count of associated models (model_count) in a single row. You can then check the model_count value to determine if the entity can be deleted.
Using a LEFT JOIN eliminates the need for a separate query and improves the efficiency of the deletion process.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2428 +/- ##
==========================================
- Coverage 68.05% 68.04% -0.01%
==========================================
Files 364 364
Lines 47875 47880 +5
==========================================
Hits 32579 32579
- Misses 15296 15301 +5 ☔ View full report in Codecov by Sentry. |
Larkooo
force-pushed
the
delete-queue
branch
from
7b6dc51
to
e62d59b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
crates/torii/core/src/query_queue.rs (1)
100-138: Ohayo, sensei! The implementation of theDeleteEntityvariant looks solid.The code segment handles the execution of the entity deletion query effectively. It first executes the deletion query and checks if any rows were affected. If no rows were affected, it continues to the next query in the queue, avoiding unnecessary processing.
If rows were affected, it updates the
entitiestable with the current timestamp, event ID, and entity ID. It then retrieves the count of associated models for the entity. If the count is zero, indicating no associated models, the entity is deleted from the database. Finally, it publishes aBrokerMessage::EntityUpdatedto reflect the changes.This implementation ensures that the entity is only deleted if all its associated models are removed, maintaining data integrity.
To improve readability, consider extracting the SQL queries into separate constants or variables with descriptive names. This will make the code more maintainable and easier to understand.
For example:
const UPDATE_ENTITY_QUERY: &str = "UPDATE entities SET updated_at=CURRENT_TIMESTAMP, executed_at=?, event_id=? WHERE id = ? RETURNING *"; const COUNT_ENTITY_MODELS_QUERY: &str = "SELECT count(*) FROM entity_model WHERE entity_id = ?"; const DELETE_ENTITY_QUERY: &str = "DELETE FROM entities WHERE id = ?";Then, you can use these constants in the corresponding
sqlx::querycalls.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- crates/torii/core/src/processors/store_del_record.rs (1 hunks)
- crates/torii/core/src/query_queue.rs (2 hunks)
- crates/torii/core/src/sql.rs (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- crates/torii/core/src/processors/store_del_record.rs
Additional comments not posted (7)
crates/torii/core/src/query_queue.rs (3)
40-46: Ohayo, sensei! The newDeleteEntityQuerystruct looks good.The struct encapsulates the necessary data for handling entity deletion requests, including the
entity_id,event_id,block_timestamp, and theentityitself. This provides a clean and structured way to pass the required information to theDeleteEntityvariant in theQueryTypeenum.
51-51: Ohayo, sensei! The newDeleteEntityvariant in theQueryTypeenum is a great addition.The
DeleteEntityvariant, which takes theDeleteEntityQuerystruct as a parameter, allows theQueryQueueto handle entity deletion requests effectively. By encapsulating the necessary data within theDeleteEntityQuerystruct, it ensures that all the required information is available for the deletion process.This variant expands the capabilities of the
QueryTypeenum and provides a clean way to manage entity deletions within the query queue system.
104-106: Ohayo, sensei! The check for affected rows is a nice optimization.By checking if the
delete_modelquery affected any rows and continuing to the next query if no rows were affected, you avoid unnecessary processing and improve the efficiency of the deletion process.If there are no associated models to delete, there is no need to proceed with updating the
entitiestable or checking the count of associated models. This optimization ensures that the code only performs the necessary steps when there are actual changes to be made.crates/torii/core/src/sql.rs (4)
Line range hint
283-302: Ohayo sensei! The entity deletion logic looks spot on.The function correctly handles the deletion of an entity and its associated data by:
- Recursively building delete queries for the entity data
- Deleting the entity-model association with the appropriate
QueryTypevariant- Storing the necessary entity details in the
DeleteEntityQuerystruct
Line range hint
769-838: The recursive delete query building logic is implemented correctly, sensei!The function handles various entity types and correctly generates delete queries by:
- Deleting the current table based on the
entity_id- Recursively processing nested types for
Struct,Tuple,Enum, andArray- Skipping empty tuple options for
Enumtypes- Handling array elements recursively
Line range hint
769-778: TheTy::Structmatch arm looks good, sensei!It correctly handles the deletion of struct entities by:
- Generating a delete query for the current table using the
table_idandentity_id- Enqueueing the query with
QueryType::Other- Recursively calling
build_delete_entity_queries_recursivefor each struct member with the updated path
Line range hint
790-808: TheTy::Enummatch arm is implemented correctly, sensei!It handles the deletion of enum entities by:
- Generating a delete query for the current table using the
table_idandentity_id- Enqueueing the query with
QueryType::Other- Skipping enum options with empty tuples
- Recursively calling
build_delete_entity_queries_recursivefor non-empty enum options with the updated path
| let count = sqlx::query_scalar::<_, i64>( | ||
| "SELECT count(*) FROM entity_model WHERE entity_id = ?", | ||
| ) | ||
| .bind(entity_updated.id.clone()) | ||
| .fetch_one(&mut *tx) | ||
| .await?; |
There was a problem hiding this comment.
To have the code organized around that we should move (in the future, not this PR) to some functions that could make this less verbose and easier to re-use, especially counting or generic stuff like so.
Summary by CodeRabbit
New Features
delete_entitymethod to accept additional parameters for more precise entity handling.Bug Fixes