feat(torii-grpc): correct keys clause models predicate #3000
Conversation
|
Ohayo sensei! WalkthroughThis PR refines the querying logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DojoWorld
participant Database
Client->>DojoWorld: query_by_keys(keys_clause)
DojoWorld->>DojoWorld: Compute model_selectors from keys_clause
alt model_selectors not empty
DojoWorld->>DojoWorld: Build where_clause with model check
else
DojoWorld->>DojoWorld: Build default where_clause
end
DojoWorld->>Database: Execute query with where_clause
Database-->>DojoWorld: Return results
DojoWorld-->>Client: Return results
sequenceDiagram
participant DojoWorld
participant ClauseBuilder
DojoWorld->>ClauseBuilder: build_composite_clause(keys, model_relation_table)
ClauseBuilder->>ClauseBuilder: Evaluate key type & model_selectors
alt model_selectors exist
ClauseBuilder->>ClauseBuilder: Combine key condition with model check
else
ClauseBuilder->>ClauseBuilder: Build clause using keys only
end
ClauseBuilder-->>DojoWorld: Return composite clause
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
🧹 Nitpick comments (2)
crates/torii/grpc/src/server/mod.rs (2)
460-460: Ohayo sensei, watch out for line length warnings.
As flagged in the pipeline failures, lines 460 and 467 exceed recommended length. Consider breaking them for better readability.Also applies to: 467-467
🧰 Tools
🪛 GitHub Actions: ci
[warning] 460-460: Line exceeds recommended length. Consider breaking the line for better readability.
1207-1207: Ohayo sensei, line length warning.
Line 1207 also exceeds recommended length. Consider splitting it into multiple lines to comply with style guidelines.🧰 Tools
🪛 GitHub Actions: ci
[warning] 1207-1207: Line exceeds recommended length. Consider breaking the line for better readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/grpc/src/server/mod.rs(5 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/grpc/src/server/mod.rs
[warning] 460-460: Line exceeds recommended length. Consider breaking the line for better readability.
[warning] 467-467: Line exceeds recommended length. Consider breaking the line for better readability.
[warning] 1207-1207: Line exceeds recommended length. Consider breaking the line for better readability.
🔇 Additional comments (4)
crates/torii/grpc/src/server/mod.rs (4)
448-449: Ohayo sensei, nice introduction of model_selectors!
No issues spotted here, it looks neat and well-structured.
725-725: Ohayo sensei, everything looks consistent!
The extra parameter for build_composite_clause is correctly passed, aiding more refined query building.
451-474: Ohayo sensei, verify the OR logic in the WHERE clause.
Because the condition includes "… OR {model_relation_table}.model_id NOT IN ({models})", rows with a model_id outside the specified set might still pass. If that’s intended, great. Otherwise, consider using a stricter check to avoid unintentionally including all models not in the set.Below is a potential fix if you want to ensure the row only matches when the model belongs to the specified set:
- "({table}.keys REGEXP ? AND {model_relation_table}.model_id IN ({models})) OR {model_relation_table}.model_id NOT IN ({models})" + "{table}.keys REGEXP ? AND {model_relation_table}.model_id IN ({models})"🧰 Tools
🪛 GitHub Actions: ci
[warning] 460-460: Line exceeds recommended length. Consider breaking the line for better readability.
[warning] 467-467: Line exceeds recommended length. Consider breaking the line for better readability.
1204-1216: Ohayo sensei, double-check the OR condition here as well.
Similar to lines 451-474, “OR {model_relation_table}.model_id NOT IN” may inadvertently match all rows that do not belong to the specified models. Confirm if that’s the intended behavior.- "({table}.keys REGEXP ? AND {model_relation_table}.model_id IN ({models})) OR {model_relation_table}.model_id NOT IN ({models})" + "{table}.keys REGEXP ? AND {model_relation_table}.model_id IN ({models})"🧰 Tools
🪛 GitHub Actions: ci
[warning] 1207-1207: Line exceeds recommended length. Consider breaking the line for better readability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/grpc/src/server/mod.rs (2)
448-481: Ohayo! The query construction looks good but could be optimized sensei!The implementation correctly handles model filtering and prevents SQL injection through proper use of bind values. However, we could optimize the model selectors handling:
- We're extending bind values twice with the same model selectors
- The SQL query could be simplified when there are no model selectors
Consider this optimization:
- if !model_selectors.is_empty() { - bind_values.extend(model_selectors.clone()); - bind_values.extend(model_selectors); - } + if !model_selectors.is_empty() { + // Add model selectors once and reuse the same bind values + let model_placeholders = vec!["?"; model_selectors.len()].join(", "); + bind_values.extend(model_selectors); + }
1208-1227: Similar optimization opportunity here sensei!The implementation mirrors the changes in
query_by_keys, with the same opportunity for optimization in model selectors handling. The security aspect is well-handled with proper bind value usage.Consider applying the same optimization as suggested for
query_by_keys:- bind_values.extend(model_selectors.clone()); - bind_values.extend(model_selectors); + // Add model selectors once and reuse the same bind values + let model_placeholders = vec!["?"; model_selectors.len()].join(", "); + bind_values.extend(model_selectors);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/grpc/src/server/mod.rs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: clippy
- GitHub Check: build
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3000 +/- ##
==========================================
- Coverage 57.10% 57.08% -0.03%
==========================================
Files 429 429
Lines 56868 56910 +42
==========================================
+ Hits 32475 32487 +12
- Misses 24393 24423 +30 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit