feat(torii-grpc): ordering and pagination #2765
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 buf (1.47.2)crates/torii/grpc/proto/types.protoConfig file YAML parsing error: yaml: unmarshal errors: WalkthroughOhayo, sensei! This pull request introduces several enhancements to the querying capabilities of the system. Key changes include the addition of new message types and enumerations in the 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)
🪧 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
🧹 Outside diff range and nitpick comments (1)
crates/torii/grpc/proto/types.proto (1)
72-80: Ohayo! Consider enhancing field documentationThe Pagination message structure looks good, but the comments could be more descriptive to help other developers understand the constraints and usage patterns.
Consider adding more detailed comments:
message Pagination { - // Updated at time-based pagination + // Unix timestamp in seconds. If specified, returns records updated before this time uint64 updated_before = 1; - uint64 updated_after = 2; + // Unix timestamp in seconds. If specified, returns records updated after this time + uint64 updated_after = 2; - // Standard offset pagination + // Maximum number of records to return (1-1000) uint32 limit = 3; + // Number of records to skip uint32 offset = 4; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/torii/grpc/proto/types.proto(2 hunks)
🔇 Additional comments (2)
crates/torii/grpc/proto/types.proto (2)
185-187: LGTM! Clean enum implementation
The OrderDirection enum follows protobuf best practices and provides clear, standard sorting options.
179-183: Verify model and member field validation
The OrderBy message structure looks clean, sensei! However, we should ensure that the specified model and member combinations are validated to prevent runtime errors.
Let's check if there's any validation logic:
✅ Verification successful
Model and member field validation is handled in GraphQL layer
Ohayo sensei! After reviewing the codebase, I found that the OrderBy message is properly validated in the GraphQL implementation layer. The validation and parsing of order fields is handled in torii/graphql/src/object/inputs/order_input.rs which ensures:
- Model fields are validated through type-safe GraphQL schema generation
- Member fields are validated through the OrderInputObject implementation
- Direction is enforced through a proper enum type
The implementation follows a clean architecture where:
- The protobuf message defines the structure
- The GraphQL layer handles the validation and parsing
- The query layer (
query::order) implements the actual ordering logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for validation logic related to OrderBy
rg -t rust "OrderBy.*validate|validate.*OrderBy"
# Search for model definitions to cross-reference available fields
ast-grep --pattern 'message $model {
$$$
}'
Length of output: 98
Script:
#!/bin/bash
# Search for OrderBy usage in Rust files
rg -t rust "OrderBy" -A 5 -B 5
# Look for validation or error handling related to sorting
rg -t rust "sort.*error|sort.*valid" -A 5 -B 5
# Find implementations or trait bounds related to OrderBy
ast-grep --pattern 'impl $_ for OrderBy'
# Search for sorting-related validation in the codebase
rg -t rust "sort.*validate|order.*validate"
Length of output: 168
Script:
#!/bin/bash
# Look for the proto file imports and usage
rg -g "*.rs" "types\.proto|\.proto.*OrderBy" -A 5 -B 5
# Find any sorting or ordering related code
rg -g "*.rs" "fn.*sort|fn.*order" -A 5 -B 5
# Look for any error handling or validation in the generated protobuf code
fd -e rs | xargs cat | rg "pub struct OrderBy|pub enum OrderDirection" -A 10 -B 2
# Search for query building or execution code that might handle ordering
rg -g "*.rs" "query.*order|order.*by" -A 5 -B 5
Length of output: 23519
crates/torii/grpc/proto/types.proto
Outdated
|
|
||
| message Query { | ||
| Clause clause = 1; | ||
| uint32 limit = 2; | ||
| uint32 offset = 3; | ||
| bool dont_include_hashed_keys = 4; | ||
| bool dont_include_hashed_keys = 2; | ||
| repeated OrderBy order_by = 3; | ||
| Pagination pagination = 4; |
There was a problem hiding this comment.
💡 Codebase verification
Breaking change impact is limited to TypeScript bindings
Ohayo sensei! After verification, the pagination changes only affect TypeScript bindings in crates/dojo/bindgen. The Query message is primarily used in:
- world.proto for RPC calls
- TypeScript bindings that have default limit/offset parameters
The changes needed are:
- Update TypeScript bindings in
crates/dojo/bindgen/src/plugins/typescript_v2/mod.rs - Update corresponding test files in
crates/dojo/bindgen/src/test_data/mocks/
🔗 Analysis chain
Verify impact of pagination field changes on existing clients
The Query message structure has been updated to use the new Pagination message. While this is a good architectural change, it's a breaking change that needs careful consideration.
Let's check for existing usage patterns:
Consider:
- Implementing a migration strategy for existing clients
- Adding deprecation notices in client SDKs
- Documenting the migration path in release notes
Let me search for more specific patterns to understand the impact:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Query message usage in proto files and client code
rg -t proto -t rust "Query.*limit|Query.*offset"
Length of output: 80
Script:
#!/bin/bash
# Search for Query usage patterns in all files
rg "Query.*limit|Query.*offset" --type-add 'proto:*.proto'
# Also check for direct Query message usage
rg "Query\s+\w+" --type-add 'proto:*.proto'
# Look for Pagination message definition and usage
rg "Pagination" --type-add 'proto:*.proto'
Length of output: 1407
Script:
#!/bin/bash
# Look for any RPC methods using Query
ast-grep --pattern 'rpc $_ (Query) returns'
# Find all proto files to manually inspect
fd '\.proto$'
Length of output: 247
Script:
#!/bin/bash
# Check git history for the specific file to understand when these fields were introduced
git log --oneline crates/torii/grpc/proto/types.proto
Length of output: 0
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
crates/torii/grpc/proto/types.proto (1)
175-183: Consider Documenting the NewOrderByMessage andOrderDirectionEnumOhayo sensei! The introduction of
OrderByandOrderDirectionenhances query capabilities. Please ensure that these new message types are properly documented in your API documentation to help users understand how to use them effectively.crates/torii/core/src/model.rs (2)
Line range hint
124-133: Update Documentation forbuild_sql_queryFunction Signature ChangeOhayo sensei! The
build_sql_queryfunction signature has been updated to include theorder_byparameter. Please ensure that any documentation or comments related to this function are updated to reflect this change.
496-496: Update Test Cases to Reflectorder_byParameter AdditionOhayo sensei! In the
struct_ty_to_querytest, the call tobuild_sql_querynow includesNonefor theorder_byparameter. Ensure that all test cases are updated accordingly, and consider adding new tests to cover scenarios with theorder_byparameter.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
crates/torii/core/src/model.rs(3 hunks)crates/torii/core/src/types.rs(1 hunks)crates/torii/grpc/proto/types.proto(2 hunks)crates/torii/grpc/src/server/mod.rs(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/torii/core/src/types.rs
🔇 Additional comments (1)
crates/torii/grpc/proto/types.proto (1)
74-82: Ensure Backward Compatibility with Updated Query Message
Ohayo sensei! The addition of order_by, updated_before, and updated_after fields to the Query message may introduce backward compatibility issues with existing clients or services that use this message. Please ensure that these changes are backward compatible or provide a migration strategy for clients.
Run the following script to check for usages of Query and identify potential impacts:
crates/torii/grpc/src/server/mod.rs
Outdated
| where_clause: Option<&str>, | ||
| order_by: Option<&str>, |
There was a problem hiding this comment.
Potential SQL Injection Vulnerability: Validate where_clause Parameter
Ohayo sensei! Passing the where_clause parameter directly into SQL query strings without proper sanitization can lead to SQL injection vulnerabilities. Please ensure that where_clause is properly validated or use parameterized queries to prevent SQL injection attacks.
Apply this diff to use parameterized queries:
- let where_clause = where_clause.map_or(
- "[{table}].id IN (SELECT id FROM temp_entity_ids WHERE model_group = ?)",
- |clause| &format!("{} AND [{table}].id IN (SELECT id FROM temp_entity_ids WHERE model_group = ?)", clause),
- );
+ let mut where_conditions = vec![];
+ let mut query_params = vec![];
+
+ // Always include the model group condition
+ where_conditions.push(format!("[{table}].id IN (SELECT id FROM temp_entity_ids WHERE model_group = ?)"));
+ query_params.push(models_str.clone());
+
+ // Validate and include the where_clause if present
+ if let Some(clause) = where_clause {
+ if is_valid_clause(clause) {
+ where_conditions.push(clause.to_string());
+ } else {
+ return Err(Error::InvalidParameter("where_clause".into()));
+ }
+ }
+
+ let where_clause = where_conditions.join(" AND ");Also applies to: 298-302
crates/torii/grpc/src/server/mod.rs
Outdated
| @@ -377,6 +383,7 @@ | |||
| limit: Option<u32>, | |||
| offset: Option<u32>, | |||
| dont_include_hashed_keys: bool, | |||
| where_clause: Option<&str>, | |||
There was a problem hiding this comment.
Potential SQL Injection Vulnerability: Validate Input Parameters
Ohayo sensei! Passing user-provided where_clause, order_by, updated_before, and updated_after parameters directly into SQL queries can lead to SQL injection vulnerabilities. Please ensure that these parameters are properly sanitized or use parameterized queries.
Consider modifying the code to use parameterized queries for these parameters and validate their values before incorporating them into SQL statements.
Also applies to: 475-477
crates/torii/grpc/src/server/mod.rs
Outdated
| let order_by = query | ||
| .order_by | ||
| .map(|order_by| { | ||
| format!( | ||
| "[{}] [{}] {}", | ||
| order_by.model, order_by.member, order_by.direction | ||
| ) | ||
| }); | ||
|
|
There was a problem hiding this comment.
Potential SQL Injection Risk in retrieve_entities Method
Ohayo sensei! The order_by parameter is constructed using string formatting and injected directly into the SQL query, which can introduce SQL injection vulnerabilities. Please validate the order_by parameter against a whitelist of allowed values or use parameterized queries to prevent SQL injection attacks.
Consider the following refactor to validate order_by:
let order_by = query
.order_by
.map(|order_by| {
+ // Validate that model and member are valid identifiers
+ if !is_valid_identifier(&order_by.model) || !is_valid_identifier(&order_by.member) {
+ return Err(QueryError::InvalidOrderByParameter);
+ }
format!(
"[{}].[{}] {}",
order_by.model, order_by.member, order_by.direction
)
});Implement a helper function is_valid_identifier to ensure that only allowed column names are used.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
crates/torii/grpc/src/server/mod.rs (1)
939-958: Consider enhancing error handling for order_by validation.The error handling could be more specific for order_by validation failures. Consider adding dedicated error variants.
// Add to the Error enum: pub enum Error { // ... existing variants ... InvalidOrderBy { model: String, member: String, reason: String, }, } // Then use it in the validation: if !is_valid_identifier(&order_by.model) { return Err(Error::InvalidOrderBy { model: order_by.model.clone(), member: order_by.member.clone(), reason: "Invalid model identifier".into(), }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/torii/grpc/proto/types.proto(3 hunks)crates/torii/grpc/src/server/mod.rs(22 hunks)crates/torii/grpc/src/types/mod.rs(2 hunks)
🔇 Additional comments (7)
crates/torii/grpc/proto/types.proto (3)
175-179: LGTM! Well-structured message and enum definitions.
The OrderBy message and OrderDirection enum are well-designed with clear field names and appropriate types.
Also applies to: 181-183
95-98: LGTM! Well-structured time filtering message.
The TimeClause message uses appropriate uint64 types for timestamp fields.
77-77: LGTM! Well-integrated ordering capability.
The addition of repeated OrderBy order_by to the Query message enables flexible multi-column sorting.
crates/torii/grpc/src/types/mod.rs (2)
82-93: LGTM! Clean Rust implementations of protobuf types.
The OrderBy struct and OrderDirection enum are well-implemented with proper trait derivations and proto conversion logic.
Also applies to: 95-99
107-107: LGTM! Proper Query struct extension.
The addition of the order_by field and its proto conversion implementation are correct.
Also applies to: 271-271
crates/torii/grpc/src/server/mod.rs (2)
Line range hint 228-238: LGTM! Consistent propagation of ordering parameter.
The order_by parameter is properly propagated through all query methods, maintaining a consistent interface.
Also applies to: 263-263, 384-384, 479-479, 649-649, 739-739
697-699:
SQL injection vulnerability in member clause construction.
The direct string interpolation of model and member values into the WHERE clause is unsafe.
Apply similar validation as suggested for the order_by clause:
- let where_clause =
- format!("[{}].[{}] {comparison_operator} ?", member_clause.model, member_clause.member);
+ if !is_valid_identifier(&member_clause.model) || !is_valid_identifier(&member_clause.member) {
+ return Err(Error::InvalidParameter("Invalid model or member name".into()));
+ }
+ let where_clause = format!(
+ "[{}].[{}] {comparison_operator} ?",
+ sanitize_identifier(&member_clause.model),
+ sanitize_identifier(&member_clause.member)
+ );Likely invalid or redundant comment.
| let order_by = query | ||
| .order_by | ||
| .iter() | ||
| .map(|order_by| { | ||
| format!( | ||
| "[{}].[{}] {}", | ||
| order_by.model, | ||
| order_by.member, | ||
| match order_by.direction { | ||
| 0 => "ASC", | ||
| 1 => "DESC", | ||
| _ => unreachable!(), | ||
| } | ||
| ) | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| .join(", "); | ||
|
|
There was a problem hiding this comment.
Ohayo sensei! SQL injection vulnerability in order_by clause construction.
The direct string interpolation of model and member values into the SQL ORDER BY clause is unsafe. An attacker could inject malicious SQL through these fields.
Consider this safer approach:
- format!(
- "[{}].[{}] {}",
- order_by.model,
- order_by.member,
- match order_by.direction {
- 0 => "ASC",
- 1 => "DESC",
- _ => unreachable!(),
- }
- )
+ // Validate model and member names against allowed patterns
+ if !is_valid_identifier(&order_by.model) || !is_valid_identifier(&order_by.member) {
+ return Err(Error::InvalidParameter("Invalid model or member name".into()));
+ }
+ // Use parameterized queries or whitelist validation
+ format!(
+ "[{}].[{}] {}",
+ sanitize_identifier(&order_by.model),
+ sanitize_identifier(&order_by.member),
+ match order_by.direction {
+ 0 => "ASC",
+ 1 => "DESC",
+ _ => unreachable!(),
+ }
+ )Add these helper functions:
fn is_valid_identifier(s: &str) -> bool {
// Only allow alphanumeric and underscore
s.chars().all(|c| c.is_alphanumeric() || c == '_')
}
fn sanitize_identifier(s: &str) -> String {
// Remove any non-alphanumeric characters except underscore
s.chars()
.filter(|c| c.is_alphanumeric() || *c == '_')
.collect()
}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
crates/torii/grpc/src/server/mod.rs (2)
Line range hint
220-229: Add documentation for the order_by parameter.Ohayo sensei! The new
order_byparameter has been added to multiple methods, but it lacks documentation explaining its format and usage.Add documentation for the
order_byparameter in each method:/// @param order_by Optional SQL ORDER BY clause in the format "[model].[member] ASC|DESC". /// Multiple ordering criteria can be specified by separating them with commas. /// Example: "ns-Position.x ASC, ns-Moves.remaining DESC"Also applies to: 264-264, 385-385, 480-480
940-959: Improve error handling for invalid order_by values.Ohayo sensei! The current implementation doesn't provide specific error handling for invalid order_by values, which could lead to confusing error messages.
Consider adding a dedicated error variant and validation:
#[derive(Debug)] pub enum OrderByError { InvalidModel(String), InvalidMember(String), InvalidDirection(i32), } impl From<OrderByError> for Error { fn from(err: OrderByError) -> Self { Error::InvalidParameter(format!("Invalid order_by: {err:?}")) } } fn validate_order_by(order: &proto::types::OrderBy) -> Result<(), OrderByError> { if !is_valid_identifier(&order.model) { return Err(OrderByError::InvalidModel(order.model.clone())); } if !is_valid_identifier(&order.member) { return Err(OrderByError::InvalidMember(order.member.clone())); } match order.direction { 0 | 1 => Ok(()), d => Err(OrderByError::InvalidDirection(d)), } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
crates/torii/grpc/src/server/mod.rs(23 hunks)crates/torii/grpc/src/server/tests/entities_test.rs(1 hunks)
🔇 Additional comments (1)
crates/torii/grpc/src/server/mod.rs (1)
940-959:
Critical: SQL Injection vulnerability in order_by clause construction.
Ohayo sensei! The direct string interpolation of model and member values into the SQL ORDER BY clause is unsafe. An attacker could inject malicious SQL through these fields.
Apply this safer approach:
- let order_by = query
- .order_by
- .iter()
- .map(|order_by| {
- format!(
- "[{}].[{}] {}",
- order_by.model,
- order_by.member,
- match order_by.direction {
- 0 => "ASC",
- 1 => "DESC",
- _ => unreachable!(),
- }
- )
- })
- .collect::<Vec<_>>()
- .join(", ");
+ let order_by = query
+ .order_by
+ .iter()
+ .map(|order_by| {
+ // Validate model and member names
+ if !is_valid_identifier(&order_by.model) || !is_valid_identifier(&order_by.member) {
+ return Err(Error::InvalidParameter("Invalid model or member name".into()));
+ }
+
+ Ok(format!(
+ "[{}].[{}] {}",
+ sanitize_identifier(&order_by.model),
+ sanitize_identifier(&order_by.member),
+ match order_by.direction {
+ 0 => "ASC",
+ 1 => "DESC",
+ _ => unreachable!(),
+ }
+ ))
+ })
+ .collect::<Result<Vec<_>, Error>>()?
+ .join(", ");Add these helper functions:
fn is_valid_identifier(s: &str) -> bool {
// Only allow alphanumeric and underscore
s.chars().all(|c| c.is_alphanumeric() || c == '_')
}
fn sanitize_identifier(s: &str) -> String {
// Remove any non-alphanumeric characters except underscore
s.chars()
.filter(|c| c.is_alphanumeric() || *c == '_')
.collect()
}| @@ -141,6 +141,7 @@ async fn test_entities_queries(sequencer: &RunnerCtx) { | |||
| Some(1), | |||
| None, | |||
| false, | |||
| None, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for the new ordering functionality.
Ohayo sensei! While the test has been updated to accommodate the new order_by parameter, it would be beneficial to add test cases that verify the ordering functionality.
Consider adding these test scenarios:
- Ordering by a single field in ascending order
- Ordering by a single field in descending order
- Ordering by multiple fields
- Ordering with null values
// Example test case for ordering
let entities = grpc
.query_by_keys(
"entities",
"entity_model",
"internal_entity_id",
&KeysClause {
keys: vec![account.address().to_bytes_be().to_vec()],
pattern_matching: 0,
models: vec![],
},
Some(1),
None,
false,
Some("ns-Position.x ASC, ns-Position.y DESC"),
)
.await
.unwrap()
.0;
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2765 +/- ##
==========================================
- Coverage 55.51% 55.46% -0.05%
==========================================
Files 434 434
Lines 54295 54356 +61
==========================================
+ Hits 30140 30150 +10
- Misses 24155 24206 +51 ☔ View full report in Codecov by Sentry. |
* proto orderb y and pagination * c * start uupdating grpc for order and updated filter * finish up queries for grpc * remove time stuff & finish ordering * fix clippy * remove time clause
Summary by CodeRabbit
Release Notes
New Features
OrderBystructure.TimeClausemessage.Bug Fixes
These updates enhance user experience by providing more detailed and customizable query options.