feat(grpc): add entity models for returned models #2805
Conversation
WalkthroughOhayo, sensei! This pull request introduces a new repeated string field, Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (2)
crates/torii/grpc/proto/types.proto(1 hunks)crates/torii/grpc/src/server/mod.rs(18 hunks)
🔇 Additional comments (2)
crates/torii/grpc/src/server/mod.rs (1)
737-743: Avoid using unwrap() to prevent potential panics, sensei
Same issue as earlier, avoid using unwrap() on Felt::from_str(id) to prevent potential panics.
crates/torii/grpc/proto/types.proto (1)
78-78: Looks good, sensei!
No issues found with the addition of the entity_models field to the Query message.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2805 +/- ##
==========================================
- Coverage 55.74% 55.74% -0.01%
==========================================
Files 439 439
Lines 55572 55613 +41
==========================================
+ Hits 30981 31001 +20
- Misses 24591 24612 +21 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/torii/grpc/src/server/mod.rs (1)
271-271: Refactor entity model ID parsing into a helper function, senseiThe code for converting
entity_modelsintoFeltselectors is repeated in multiple places. Consider refactoring this logic into a helper function to improve maintainability.Also applies to: 703-704, 745-746
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/torii/grpc/src/server/mod.rs(18 hunks)crates/torii/grpc/src/server/tests/entities_test.rs(1 hunks)crates/torii/grpc/src/types/mod.rs(2 hunks)
🔇 Additional comments (8)
crates/torii/grpc/src/server/mod.rs (5)
16-16: Great addition of the import, sensei!
The import of compute_selector_from_tag strengthens the code's functionality.
231-231: Method signature enhancement acknowledged, sensei
Adding entity_models: Vec<String> to entities_all improves entity filtering capabilities.
268-272: Consistent conversion of entity models, sensei
Converting entity_models to Felt selectors is necessary. Ensure consistency across methods.
325-334: Avoid panics by handling potential errors when parsing model IDs, sensei
Using unwrap() on Felt::from_str(id) can cause a panic if id is invalid. It's better to handle the possible Err case to prevent runtime panics.
744-752:
Handle potential from_str errors to prevent panics, sensei
Using unwrap() on Felt::from_str(id) may cause panics with invalid strings. Consider error handling to ensure robustness.
Apply this diff to safely handle parsing errors:
-let model_id = Felt::from_str(id).unwrap();
+let model_id = match Felt::from_str(id) {
+ Ok(model_id) => model_id,
+ Err(_) => {
+ // Handle the error, e.g., skip this ID or log the error
+ return Ok(None);
+ }
+};Likely invalid or redundant comment.
crates/torii/grpc/src/server/tests/entities_test.rs (1)
145-145: Ensuring default entity models in tests, sensei
Passing an empty vector vec![] for entity_models aligns with the updated method signature. Ensure that tests cover cases with and without specified entity models.
crates/torii/grpc/src/types/mod.rs (2)
108-108: Addition of entity_models field strengthens Query struct, sensei
Including entity_models: Vec<String> in the Query struct enhances query capabilities. Ensure serialization and deserialization handle this new field appropriately.
273-273: Ensure proper mapping of entity_models in From implementation, sensei
Mapping entity_models in the From<Query> implementation ensures consistency between internal and external representations.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation