feat(torii-grpc): cursor based pagination #3147
Conversation
WalkthroughOhayo, sensei! This update introduces significant changes to the Protocol Buffers definition and the querying logic in the DojoWorld implementation. The Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant S as GRPC Server
participant W as DojoWorld
participant DB as Database
C->>S: Send query request (with optional pagination)
S->>W: Call DojoWorld method with pagination parameter
W->>W: Build query ("WHERE id > cursor" if provided)
W->>DB: Execute query
DB-->>W: Return paginated results
W-->>S: Return results
S-->>C: Send response with results and cursor info (if applicable)
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)
⏰ Context from checks skipped due to timeout of 90000ms (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
🧹 Nitpick comments (1)
crates/torii/grpc/src/server/mod.rs (1)
1000-1005: Ohayo sensei, thanks for addingcursorchecks inretrieve_entities.
Remember the same numeric vs. string mismatch concerns apply if the ID isn't zero-padded or stored numerically.Also applies to: 1018-1018, 1043-1043, 1059-1059, 1075-1075, 1091-1091
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/torii/grpc/proto/types.proto(1 hunks)crates/torii/grpc/src/server/mod.rs(17 hunks)crates/torii/grpc/src/types/mod.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (8)
crates/torii/grpc/proto/types.proto (1)
80-80: Ohayo sensei, the newcursorfield addition looks straightforward!
This change cleanly enables pagination through a bytes-based cursor.crates/torii/grpc/src/types/mod.rs (2)
136-137: Ohayo sensei, great job adding thecursorfield to enable pagination.
Having a strongly typedOption<Felt>is a clean way to manage cursors.
311-311: Ohayo sensei, the conversion logic forcursorlooks good.
Converting the optionalFeltinto a byte vector or an empty vector whenNonematches the new proto design.crates/torii/grpc/src/server/mod.rs (5)
275-275: Ohayo sensei, nice addition ofcursorparameter toentities_all.
This extension to support cursor-based pagination will be helpful for large datasets. Just ensure your sorting column is consistently numeric or zero-padded so the pagination is correct.Also applies to: 288-288
494-516: Ohayo sensei, the same potential issue applies inquery_by_keys.
Comparisons withAND {table}.id > ?should be verified ifidis stored in a format where lexicographical ordering equals numeric ordering. If not, consider zero-padding all hex IDs or using a numeric column.
776-784: Ohayo sensei, good job includingcursorinquery_by_composite.
Again, confirm the stored ID format matches the numeric-based filtering you desire.
1229-1229: Ohayo sensei, nice to see the cursor logic integrated intobuild_composite_clause.
Just ensure theid > cursorcomparison doesn't break numeric pagination if youridcolumn is stored in text form.Also applies to: 1335-1343
355-364:❓ Verification inconclusive
Ohayo sensei, watch out for string vs. numeric ordering with
cursorchecks.
Inquery_by_hashed_keys, lines likeAND {table}.id > ?may lead to unexpected results if theidcolumn is stored as a text type. If your IDs vary in hex length, strictly lexicographical comparison can break expected numeric ordering.Would you like to verify the column type and zero-padding logic? For example, here's a shell script to search your migrations or DDL for how
idis created:Also applies to: 366-374, 377-384
🏁 Script executed:
#!/bin/bash # Searching references to table creation or 'id' column type for potential string-based storage rg -A 10 "CREATE TABLE.*entities" rg -B 3 -A 3 "id "Length of output: 125411
Ohayo sensei – please double-check your hex formatting for ID ordering!
Our migration output confirms that the
entitiestable (and similar tables) declares theidcolumn as TEXT (e.g. incrates/torii/migrations/20231127235011_entity_model.sqland others). This means that SQL comparisons likeAND {table}.id > ?will use lexicographical ordering. If your IDs are generated viaformat!("{:#x}", …), they might vary in length unless you apply zero-padding to a fixed width. This could lead to unexpected order if, for example, a shorter hex string is considered “smaller” than a longer one even when its numeric value is greater.Please verify that your hex formatter produces consistently fixed-width (zero-padded) values or consider casting/converting the value appropriately in your query. This concern also applies to similar queries at lines 366–374 and 377–384.
crates/torii/grpc/src/server/mod.rs
Outdated
| @@ -649,6 +640,7 @@ impl DojoWorld { | |||
| order_by: Option<&str>, | |||
| entity_models: Vec<String>, | |||
| entity_updated_after: Option<String>, | |||
| cursor: Option<Felt>, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei, cursor-based filtering in query_by_member is solid, but double-check the ID comparison.
If table.id is stored as a non-zero-padded string, AND {table}.id > :cursor might not compare numerically.
Consider storing IDs as a consistently-sized hex string or numeric type to avoid ordering confusion.
Also applies to: 735-738
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/torii/sqlite/src/model.rs (2)
350-350: Consider initializing with capacity.When you know you'll be adding items to a vector, it's more efficient to initialize it with a capacity.
- let mut all_rows = Vec::new(); + let mut all_rows = Vec::with_capacity(pagination.limit.unwrap_or(100) as usize);
386-432: Cursor-based pagination implementation is robust.The implementation handles both directions of pagination and supports custom ordering when needed. The code correctly constructs SQL conditions based on cursor values and pagination direction.
However, there's potential to simplify the cursor parsing logic:
- let cursor = pagination.cursor.as_ref().unwrap(); - let cursor_values: Vec<&str> = cursor.split(',').collect(); + let cursor_values: Vec<&str> = pagination.cursor.as_ref().unwrap().split(',').collect();crates/torii/sqlite/src/types.rs (1)
290-296: Comprehensive Pagination struct.This struct nicely encapsulates all the necessary components for flexible pagination:
- Optional cursor for continuing from a specific point
- Optional limit for controlling page size
- Direction for forward/backward navigation
- Vector of OrderBy for custom sorting
Consider adding default values for common fields:
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +#[derive(Default)] pub struct Pagination { pub cursor: Option<String>, pub limit: Option<u32>, - pub direction: PaginationDirection, + pub direction: PaginationDirection, pub order_by: Vec<OrderBy>, } +impl Default for PaginationDirection { + fn default() -> Self { + Self::Forward + } +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
crates/torii/sqlite/src/model.rs(4 hunks)crates/torii/sqlite/src/types.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/torii/sqlite/src/model.rs (3)
crates/torii/sqlite/src/executor/mod.rs (8)
sqlx(312-312)sqlx(447-447)sqlx(533-533)sqlx(562-562)sqlx(627-627)sqlx(714-714)new(168-170)new(243-269)crates/torii/graphql/src/query/mod.rs (1)
e(67-81)crates/torii/sqlite/src/lib.rs (3)
e(914-918)model(513-515)new(65-72)
🪛 GitHub Actions: ci
crates/torii/sqlite/src/model.rs
[error] 1-1: Rust formatting check failed. Run 'rustfmt' to format the code according to style guidelines.
🔇 Additional comments (11)
crates/torii/sqlite/src/model.rs (7)
16-16: Ohayo! Great work on adding these new imports for pagination.The imports correctly include all necessary types for the new pagination feature.
295-297: Nice API simplification, sensei!Refactoring the function signature to use a single
Paginationparameter instead of multiple individual parameters makes the API cleaner and more maintainable. The return type change toPage<SqliteRow>nicely aligns with the cursor-based pagination approach.
312-312: Good string interpolation update.Using format string with interpolation for constructing path names is cleaner than the previous approach.
317-317: SQL injection protection improvement!Using string interpolation with square brackets for table and column names provides better protection against SQL injection. This is a security improvement over string concatenation.
Also applies to: 327-327
334-348: Well-structured order_by clause construction.The logic for building the
order_by_clausehandles both default and custom ordering gracefully. The default ordering by event_id and id ensures consistent pagination results when no specific ordering is requested.
376-379: SQL query construction looks good.The formatted SQL query properly uses the selections and joins clauses.
440-448: Pagination and ordering applied correctly.The code correctly applies the ordering and limit clauses based on the pagination parameters.
crates/torii/sqlite/src/types.rs (4)
278-282: Well-designed Page struct, sensei!The
Page<T>struct is a clean and generic implementation for paginated results. The inclusion ofnext_cursorenables efficient cursor-based pagination without requiring offset calculations.
284-288: Clear pagination direction enum.The
PaginationDirectionenum provides a semantic way to specify navigation direction in paginated results.
298-302: Standard OrderDirection enum.The enum provides clear semantics for sorting direction.
304-309: Well-structured OrderBy struct.This struct provides a clear way to specify ordering criteria with model, member, and direction.
crates/torii/sqlite/src/model.rs
Outdated
| if let Some(limit) = pagination.limit { | ||
| let requested_limit = limit - 1; // Number of rows user actually wants | ||
| let (items, next_cursor) = if all_rows.len() > requested_limit as usize { | ||
| let extra_row = all_rows.len() == limit as usize; | ||
| let items: Vec<_> = all_rows.drain(..requested_limit as usize).collect(); // Take only requested rows | ||
|
|
||
| let next_cursor = if extra_row { | ||
| let last_row = &items[items.len() - 1]; // Last row of items (not extra row) | ||
| let cursor_values = if pagination.order_by.is_empty() { | ||
| vec![ | ||
| last_row.get::<String, &str>("event_id").to_string(), | ||
| last_row.get::<String, &str>("id").to_string(), | ||
| ] | ||
| } else { | ||
| let mut values = vec![last_row.get::<String, &str>("id").to_string()]; | ||
| values.extend(pagination.order_by.iter().map(|order| { | ||
| let column = format!("{}.{}", order.model, order.member); | ||
| last_row.get::<String, &str>(&column).to_string() | ||
| })); | ||
| values | ||
| }; | ||
| Some(cursor_values.join(",")) | ||
| } else { | ||
| query += &format!(" ORDER BY {}.event_id DESC", table_name); | ||
| } | ||
| None | ||
| }; | ||
|
|
||
| if let Some(limit) = limit { | ||
| query += &format!(" LIMIT {}", limit); | ||
| match pagination.direction { | ||
| PaginationDirection::Forward => (items, next_cursor), | ||
| PaginationDirection::Backward => (items.into_iter().rev().collect(), next_cursor), | ||
| } | ||
|
|
||
| if let Some(offset) = offset { | ||
| query += &format!(" OFFSET {}", offset); | ||
| } else { | ||
| let items = all_rows; // Take all rows if fewer than requested | ||
| match pagination.direction { | ||
| PaginationDirection::Forward => (items, None), | ||
| PaginationDirection::Backward => (items.into_iter().rev().collect(), None), | ||
| } | ||
| }; | ||
|
|
||
| // Execute main query | ||
| let mut stmt = sqlx::query(&query); | ||
| for value in &bind_values { | ||
| stmt = stmt.bind(value); | ||
| } | ||
| let chunk_rows = stmt.fetch_all(pool).await?; | ||
| all_rows.extend(chunk_rows); | ||
| } | ||
| Ok(Page { items, next_cursor }) | ||
| } else { | ||
| let items = match pagination.direction { | ||
| PaginationDirection::Forward => all_rows, | ||
| PaginationDirection::Backward => all_rows.into_iter().rev().collect(), | ||
| }; | ||
| Ok(Page { items, next_cursor: None }) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Thorough handling of pagination results.
Ohayo, sensei! The code elegantly:
- Limits results to one more than requested to determine if there's a next page
- Constructs the next cursor based on the last row
- Handles result direction based on the pagination direction
One potential issue to verify:
🏁 Script executed:
#!/bin/bash
# Verify if there are any usages of the fetch_entities function that might still expect the old return type
rg -A 3 -B 3 "fetch_entities\(" --type rustLength of output: 2118
Ohayo, sensei! Update on Pagination Handling & fetch_entities Return Type
Your implementation in crates/torii/sqlite/src/model.rs (lines 459–504) for managing pagination is elegant:
- It correctly limits results to one more than requested to detect if there's a next page.
- It builds the next cursor based on the last row.
- It respects the pagination direction when returning results.
However, our verification reveals that fetch_entities is still being invoked in several places (e.g., in crates/torii/grpc/src/server/mod.rs) with tuple destructuring like:
let (rows, total_count) = fetch_entities(...);This suggests that these call sites might still be expecting the old return type. Please review and update these usages to correctly handle the new Page struct return type from fetch_entities.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
crates/torii/grpc/src/types/mod.rs (2)
320-327: Use string interpolation instead of format! when appropriateIn the
collect_columnsfunction, there are multiple uses offormat!that could be simplified using string interpolation.- let new_path = if path.is_empty() { - format!("{}", i) - } else { - format!("{}.{}", path, i) - }; + let new_path = if path.is_empty() { + i.to_string() + } else { + format!("{}.{}", path, i) + };Similarly for other format! calls with simple substitutions.
319-327: Check consistency of table and column escapingThere's inconsistency in how SQL identifiers are escaped across the codebase. In some places, square brackets are used, while in others, double quotes are used.
Consider adopting a consistent approach for SQL identifier escaping throughout the codebase, either using square brackets consistently (for SQLite) or using a helper function that handles the escaping appropriately.
crates/torii/grpc/proto/types.proto (1)
193-197: Ohayo sensei, nice to seePaginationDirectionintroduced!
Forward/backward pagination is a key improvement for flexible data retrieval. Make sure callers handle both directions consistently and confirm that the default direction is properly documented.crates/torii/grpc/src/server/mod.rs (4)
145-158: Ohayo sensei, theFrom<proto::types::Pagination>implementation is neat.
Mapping empty string toNoneforcursoris a clean approach. Also, ensuringlimit == 0gets converted toNoneis a good way to avoid confusing logic downstream. The usage ofunreachable!()is fine but remember to handle unexpected cases gracefully in production.
445-543: Ohayo sensei,query_by_keysextends consistent pagination too!
- Regex-based searching: Keep an eye out for performance overhead with the
REGEXPapproach, especially on large tables.- Model and key filtering: Combining them into the
OR/NOT INstatement is clever, but watch out for complex queries that might balloon in cost.- Pagination: The code reuses the same standardized approach for limit/cursor ordering.
You might consider using a more specialized indexing approach if performance becomes an issue.
1556-1556: Ohayo sensei, streaming response withremaining_countis a nice touch.
This extra context in the response can help clients track when they're almost done receiving data.
47-47: Ohayo sensei, formatting alert!
The pipeline warns about a formatting issue. Please runrustfmtto keep the code consistent and pass CI.🧰 Tools
🪛 GitHub Actions: ci
[warning] 47-47: Formatting issue detected. Please run 'rustfmt' to format the code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
crates/torii/grpc/proto/types.proto(2 hunks)crates/torii/grpc/proto/world.proto(1 hunks)crates/torii/grpc/src/server/mod.rs(18 hunks)crates/torii/grpc/src/types/mod.rs(3 hunks)crates/torii/sqlite/src/error.rs(1 hunks)crates/torii/sqlite/src/model.rs(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/torii/grpc/src/server/mod.rs (1)
crates/torii/sqlite/src/model.rs (20)
pagination(529-537)new(39-73)fetch_entities(290-547)row(135-135)row(139-139)row(143-143)row(147-147)row(151-151)row(160-160)row(164-164)row(168-168)row(172-172)row(182-182)row(192-192)row(200-200)row(204-204)row(212-212)row(220-220)row(228-228)i(425-430)
🪛 GitHub Actions: ci
crates/torii/grpc/src/server/mod.rs
[warning] 47-47: Formatting issue detected. Please run 'rustfmt' to format the code.
🔇 Additional comments (21)
crates/torii/sqlite/src/error.rs (1)
23-24: Ohayo, sensei! Good addition of a specific error for paginationAdding the
InvalidCursorerror variant is crucial for providing clear feedback when cursor-based pagination encounters issues with malformed or invalid cursor values.crates/torii/sqlite/src/model.rs (8)
4-5: Ohayo! Good addition of base64 dependencyAdding the base64 dependency is appropriate for encoding/decoding cursor values in a URL-safe format.
18-18: Nice comprehensive imports for pagination supportThe imports correctly include all the necessary types for the new pagination implementation, organizing them logically.
290-300: Ohayo, sensei! Function signature updated for cursor-based paginationThe function signature has been updated to accept a
Paginationparameter instead of separate pagination parameters, making the API more cohesive and extensible.Note that this change might require updates to call sites that expect the old return type (a tuple of rows and count).
352-375: Well-implemented pagination direction handlingThe code properly handles both forward and backward pagination by adjusting the order direction based on the pagination direction. This ensures consistent results regardless of pagination direction.
398-443: Ohayo! Thorough cursor parsing and condition buildingThis implementation carefully:
- Decodes the cursor using base64
- Parses the JSON values
- Builds appropriate WHERE conditions based on order direction and pagination direction
- Properly handles multi-column ordering with equality conditions for previous columns
The error handling for invalid cursors is also appropriate.
479-499: Great SQL injection prevention with parameterized queriesThe code properly binds all values from both the original bind_values and the cursor_bind_values, preventing SQL injection vulnerabilities.
508-523: Effective "peek-ahead" pagination implementationFetching one extra item to determine if there's a next page is an efficient approach. The code properly truncates the result to the original limit and handles result direction correctly.
526-546: Well-structured next cursor generationThe code efficiently:
- Checks if we have enough results to warrant a next cursor
- Extracts the relevant values from the last row based on order_by fields
- Serializes and encodes the values properly using base64
This approach ensures consistent pagination behavior.
crates/torii/grpc/src/types/mod.rs (3)
22-33: Ohayo, sensei! Well-designed pagination typesThe new
PaginationDirectionenum andPaginationstruct are well-designed for supporting cursor-based pagination. The struct includes all necessary fields (cursor, limit, direction, order_by) for implementing efficient pagination.
35-44: Clean conversion implementation for protocol buffersThe
From<Pagination>implementation correctly converts the Rust types to their protocol buffer equivalents. Usingunwrap_or_default()for the cursor ensures that null cursors are handled properly.
146-157: Improved field naming in Query structRenaming:
dont_include_hashed_keystono_hashed_keysentity_modelstomodelsThese changes improve code readability and make the API more intuitive. The comment about hashed keys is still relevant and provides useful context.
crates/torii/grpc/proto/types.proto (2)
74-76: Ohayo sensei, new pagination fields look healthy!
The addition ofno_hashed_keys,models, andpaginationcentralizes query control in one place. This improves maintainability and ensures queries are standardized. No immediate concerns here.
199-203: Ohayo sensei,Paginationstruct design is straightforward!
Includingcursor,limit, andorder_byis a standard approach. The repeatedOrderByis especially handy for multi-column ordering. Great job setting it up.crates/torii/grpc/src/server/mod.rs (7)
50-50: Ohayo sensei, good addition for pagination imports.
The direct import ofPagination,PaginationDirection, etc., simplifies usage throughout this file.
160-172: Ohayo sensei, theFrom<proto::types::OrderBy>converter is well-structured!
Mapping numeric values toOrderDirectionand usingunreachable!()is fine, though be aware of potential unknown values from older clients.
298-301: Ohayo sensei, new parameters inentities_allenhance uniformity!
Allowingpagination,no_hashed_keys, andmodelskeeps the interface symmetrical with other query methods.
356-443: Ohayo sensei,query_by_hashed_keyschanges look powerful!
- Pagination: Great job incorporating the
Paginationstruct. Having a single approach for limit/cursor looks consistent with the rest of the codebase.no_hashed_keys: The option to remove hashed keys from results is a neat privacy feature—just ensure this behavior is tested somewhere.- Multiple
models: Thehaving_clausethat checks for model presence is logical.- Cursor-based logic:
fetch_entitiesusage is consistent with your new pagination rules.
545-558: Ohayo sensei,query_by_membermethod is succinct.
Exposing a singlepaginationparameter and reusing the same fetch logic fosters DRY code. The direct mapping ofmember_clauseto SQL columns is straightforward, just ensure you handle potential name collisions or reserved keywords.
669-723: Ohayo sensei,query_by_compositeis well-handled.
The recursive approach inbuild_composite_clauseis quite flexible for complex queries. Good job reusing the same pagination approach for these advanced queries.
905-917: Ohayo sensei, requiringpaginationinretrieve_entitiesis a strong design choice.
Forcing the caller to provide pagination ensures consistent usage across all queries. This should reduce confusion and unify query patterns.
| @@ -243,7 +243,7 @@ message RetrieveEventMessagesRequest { | |||
|
|
|||
| message RetrieveEntitiesResponse { | |||
| repeated types.Entity entities = 1; | |||
| uint32 total_count = 2; | |||
| string next_cursor = 2; | |||
There was a problem hiding this comment.
Ohayo! API change from count-based to cursor-based pagination
This change from total_count to next_cursor represents a shift to more efficient cursor-based pagination, which is excellent for performance with large datasets. However, this is a breaking change for API consumers who might rely on knowing the total count of results.
Consider if you need a transitional period or additional documentation to help clients adapt to this API change. Many frontend developers rely on total counts for showing pagination UI (like "Page 1 of 10").
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
crates/torii/grpc/src/client.rs (1)
238-244: 💡 Verification agent🧩 Analysis chain
Ohayo! Similar improvement for retrieve_event_messages method
Good consistency in applying the same pattern to
retrieve_event_messagesas you did withretrieve_entities. The historical parameter has been properly moved into the Query struct.
🏁 Script executed:
#!/bin/bash # Verify all callers of retrieve_event_messages have been updated echo "Checking calls to retrieve_event_messages..." rg -A 3 -B 3 "retrieve_event_messages\(" --type rustLength of output: 2127
Ohayo, Sensei! Retrieve Event Messages Invocation Update Needed
Great work updating the implementation in
crates/torii/grpc/src/client.rsto use the Query struct—this mirrors the improvements made forretrieve_entities, and the historical parameter has indeed been encapsulated into the Query. However, I noticed that incrates/torii/client/src/client/mod.rsthe method is still being invoked with two parameters (query, historical). Please update this call to construct and pass a single Query object so that it matches the new function signature.
- Ensure that the call in
crates/torii/client/src/client/mod.rsis updated to pass one parameter (the Query struct with the historical flag included).- Verify that any other invocations of
retrieve_event_messagesfollow the new pattern for consistent behavior.Thanks, Sensei!
♻️ Duplicate comments (1)
crates/torii/grpc/proto/world.proto (1)
242-242: Ohayo! API change from count-based to cursor-based paginationThis change from
total_counttonext_cursorrepresents a shift to more efficient cursor-based pagination, which is excellent for performance with large datasets. However, this is a breaking change for API consumers who might rely on knowing the total count of results.Consider if you need a transitional period or additional documentation to help clients adapt to this API change. Many frontend developers rely on total counts for showing pagination UI (like "Page 1 of 10").
🧹 Nitpick comments (7)
crates/torii/grpc/src/server/mod.rs (7)
314-327: Document or refine the “no order by for historical entities” policy.
You're returning an error ifpagination.order_byis set for historical queries. Explain why or consider partial support if needed.
328-407: Consider indexing event_id for performance.
Ohayo sensei, for large historical queries, an index onevent_idcould speed up sorting and filtering.
409-486: Consolidate_historicalchecks across query methods.
This pattern of branching tofetch_historical_entitiesis repeated in multiple queries. Consider unifying the logic to reduce duplication.
489-569: Refactor the repeated checks into a shared helper.
Again, the_historicallogic is nearly the same as in other queries. A shared function could remove duplication.
579-693: Streamline query-by-member with other patterns.
Ohayo sensei, this code also echoes the_historicalfallback and repeated steps from other queries.
696-749: Unify composite query approach.
This code closely mirrors the same logic for fetching, pagination, and historical checks used elsewhere.
1605-1627: Clarify remaining_count usage.
Ohayo sensei, if streaming partial results with pagination, consider clarifying thatremaining_countis just for the current page items, not the total across the entire dataset.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
crates/torii/grpc/proto/types.proto(2 hunks)crates/torii/grpc/proto/world.proto(1 hunks)crates/torii/grpc/src/client.rs(1 hunks)crates/torii/grpc/src/server/mod.rs(18 hunks)crates/torii/grpc/src/types/mod.rs(3 hunks)crates/torii/sqlite/src/error.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/grpc/src/types/mod.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/torii/grpc/src/client.rs (1)
crates/torii/grpc/src/server/mod.rs (1)
retrieve_event_messages(1341-1359)
🪛 GitHub Actions: ci
crates/torii/grpc/src/server/mod.rs
[warning] 47-47: Formatting issue detected. Please run 'rustfmt' to format the code.
🔇 Additional comments (12)
crates/torii/sqlite/src/error.rs (2)
23-24: Ohayo! Good addition of specific cursor error handlingAdding a dedicated error variant for invalid cursors makes error handling more precise and improves API clarity by allowing consumers to specifically handle cursor-related issues.
49-50: Enhancing error reporting with dynamic messages, nice work sensei!Upgrading the
UnsupportedQueryvariant to accept a String parameter improves error reporting by allowing for more specific error messages tailored to each unsupported query scenario.crates/torii/grpc/proto/types.proto (3)
74-77: Ohayo! Well-structured Query message updateThe reorganization of the Query message with dedicated fields for pagination-related concerns is a good design improvement, sensei. Moving
historicalinto the Query message itself aligns with the client API changes and centralizes query parameters.
195-198: Nice addition of pagination direction enumAdding a dedicated enum for pagination direction improves type safety and API clarity. The FORWARD/BACKWARD values make the intent clear for cursor-based navigation.
200-205:✅ Verification successful
Excellent modularization with dedicated Pagination message
Centralizing pagination parameters into a dedicated message type makes the API more cohesive and follows good design principles. This structured approach will make future extensions to pagination capabilities easier, sensei.
🏁 Script executed:
#!/bin/bash # Verify that the new Pagination message is properly used in relevant files echo "Checking usage of Pagination type..." rg -A 2 -B 2 "Pagination" --type rustLength of output: 11707
Ohayo, sensei! Fantastic job on the modularization.
Your dedicatedPaginationmessage incrates/torii/grpc/proto/types.proto(lines 200-205) is spot-on. The recent verification confirms that its usage is consistently implemented across key modules—including conversion routines incrates/torii/grpc/src/types/mod.rs, query handling incrates/torii/grpc/src/server/mod.rs, and in the SQLite integration. This well-structured approach not only adheres to sound design principles but also paves the way for seamless future pagination enhancements. Keep it up, sensei!crates/torii/grpc/src/server/mod.rs (7)
50-50: No issues with the imports.
These imports appear to be used throughout the file, so everything looks good.
298-301: Function signature update looks good.
Adding pagination, no_hashed_keys, and models is coherent with the new logic.
1273-1274: Building the composite WHERE clause looks solid.
Ohayo sensei, the OR/AND logic is clear, and no issues are detected here.
1321-1339: Revised retrieve_entities is coherent.
The integration of pagination lines up perfectly with the internal query logic.
1341-1359: New pagination integration for retrieve_event_messages is good.
This parallels the retrieve_entities approach, ensuring consistent usage.
1361-1374: Revised retrieve_events with query object is fine.
No immediate concerns; the offset/limit logic remains straightforward.
47-47:❓ Verification inconclusive
Ohayo sensei, please run rustfmt.
The pipeline complains about formatting at line 47. Running rustfmt will align your code with Rust style conventions.Here is a script to ensure formatting is correct:
🏁 Script executed:
#!/bin/bash cargo fmt -- --checkLength of output: 118
Ohayo sensei, please verify rustfmt check!
It looks like our attempt to runcargo fmt -- --checkhit a snag with a "command not found" error—suggesting the Rust toolchain isn’t available in the current environment. Could you please manually verify that the code incrates/torii/grpc/src/server/mod.rs(line 47) is formatted according to Rust style conventions? Alternatively, ensure that your CI pipeline has the Rust toolchain installed so that rustfmt can run correctly.🧰 Tools
🪛 GitHub Actions: ci
[warning] 47-47: Formatting issue detected. Please run 'rustfmt' to format the code.
| let request = RetrieveEntitiesRequest { query: Some(query.into()) }; | ||
| self.inner.retrieve_entities(request).await.map_err(Error::Grpc).map(|res| res.into_inner()) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Simplified API by removing the standalone historical parameter
Moving the historical field into the Query struct itself makes the API more consistent and easier to maintain, sensei. This is a good refactoring that reduces parameter sprawl.
🏁 Script executed:
#!/bin/bash
# Verify all callers of retrieve_entities have been updated to use the Query.historical field
echo "Checking calls to retrieve_entities..."
rg -A 3 -B 3 "retrieve_entities\(" --type rustLength of output: 2065
Ohayo sensei, API refactoring requires an update to caller usage.
The refactor in crates/torii/grpc/src/client.rs—where the historical flag has been moved into the Query struct—is a great step toward simplifying the API and reducing parameter sprawl. However, our verification shows that in crates/torii/client/src/client/mod.rs the call to retrieve_entities still passes two parameters (i.e., query and a separate historical flag). Please update that caller to integrate the historical value into the Query before passing it to retrieve_entities.
- File:
crates/torii/client/src/client/mod.rs
Action: Change the call from
grpc_client.retrieve_entities(query, historical).await?
to something like
grpc_client.retrieve_entities(query_with_historical).await?,
wherequery_with_historicalis aQuerythat includes the appropriatehistoricalflag.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/torii/grpc/src/server/mod.rs (1)
149-161: 🛠️ Refactor suggestionOhayo sensei! Consider avoiding
unreachable!()for invalid pagination directions.
Right now, an unexpected direction panics. Returning an error instead prevents unwarranted panics if the client passes invalid data.Here’s a potential minimal fix:
-match value.direction { - 0 => PaginationDirection::Forward, - 1 => PaginationDirection::Backward, - _ => unreachable!(), -} +let dir = match value.direction { + 0 => PaginationDirection::Forward, + 1 => PaginationDirection::Backward, + _ => { + // Return an error or default direction + return Self { + cursor: None, + limit: None, + direction: PaginationDirection::Forward, + order_by: vec![], + }; + } +};
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/torii/grpc/Cargo.toml(1 hunks)crates/torii/grpc/src/server/mod.rs(21 hunks)crates/torii/sqlite/src/model.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (12)
crates/torii/grpc/Cargo.toml (1)
31-31: Ohayo sensei! Great addition of thebase64dependency.
This inclusion is essential for the new cursor encoding logic. No issues spotted here.crates/torii/sqlite/src/model.rs (4)
1-6: Ohayo sensei! Import statements look solid.
These newly added imports forbase64andPagination-related types facilitate cursor-based pagination. Great job.Also applies to: 19-19
301-460: Ohayo sensei! Pagination logic stands out as neat and efficient.
You're correctly handling the following:
- Distinguishing the limit and reversed rows for backward pagination.
- Building the next cursor from the last retrieved row.
- Properly chunking schemas to avoid excessive JOINs.
Everything looks logically consistent. One minor suggestion: ensure all edge cases with minimal or zero rows after filtering are tested, especially if
pagination.limitis small.
462-543: Ohayo sensei! These helper functions for cursor conditions and query building look clean.
They neatly handle the composition of various conditions (WHERE,HAVING, etc.) and safely manage cursor-based comparisons. Kudos for the careful string formatting that escapes table and column names.
545-557: Ohayo sensei! Good approach for extracting cursor values.
Using theevent_idas a fallback is a straightforward mechanism for tracking the paging offset when no ordering fields are specified. This looks sound for forward and backward direction handling. Keep it up!crates/torii/grpc/src/server/mod.rs (7)
16-17: Ohayo sensei! Imports for base64 cursor logic are correct.
No concerns: these lines properly bring in the standard Base64 engine.
53-53: Ohayo sensei! AddingTokenandTokenBalancetypes.
This import alignment is consistent with the rest of the codebase’s approach.
298-305: Ohayo sensei! Good adaptation ofentities_allto require pagination.
Ensuring direct calls to this function supply a validPaginationstruct fosters a consistent approach to retrieving data.
318-415: Ohayo sensei! Historical entities rejectorder_by, which is consistent with the logic.
This code properly returns an error ifpagination.order_byis used. Looks coherent for the domain. Good call.
417-496: Ohayo sensei!query_by_hashed_keysnicely integrates the new pagination logic.
Everything from thewhere_clausecomposition down to the chunk-based retrieval withfetch_entitiesis well-structured.
498-581: Ohayo sensei!query_by_keysseamlessly extends the same pagination approach.
The combined usage of regex for partial key matching and model filtering is well-handled. Keep it up!
583-697: Ohayo sensei!query_by_member,query_by_composite, and the final internal transformations look organized.
This set of changes harmonizes well with the code’s overall pagination structure. Nicely done.
| pagination: Pagination, | ||
| bind_values: Vec<String>, | ||
| ) -> Result<(Vec<sqlx::sqlite::SqliteRow>, u32), Error> { | ||
| // Helper function to collect columns (existing implementation) | ||
| ) -> Result<Page<SqliteRow>, Error> { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ohayo sensei! Confirm all old calls to fetch_entities are updated.
The introduction of pagination: Pagination modifies the function signature. Ensure that any existing call sites have been revised to supply a proper Pagination object.
Would you like to run a quick search to confirm that no call sites remain that pass the old parameter list?
🏁 Script executed:
#!/bin/bash
# Searching all calls to fetch_entities to ensure they have updated parameters
rg -A 3 -B 3 "fetch_entities\(" --type rustLength of output: 2009
Ohayo sensei!
Our grep search indicates that the call sites in crates/torii/grpc/src/server/mod.rs still invoke fetch_entities with only three parameters (the pool, schemas, and table) rather than the new signature that requires a Pagination object (and a Vec<String> for bind values). Please update these call sites accordingly.
- Crates/torii/grpc/src/server/mod.rs:
- Several calls to
fetch_entities(lines shown in grep output) need to be revised to include a properPaginationinstance and the correspondingbind_valuesargument.
- Several calls to
| impl From<proto::types::OrderBy> for OrderBy { | ||
| fn from(value: proto::types::OrderBy) -> Self { | ||
| OrderBy { | ||
| model: value.model, | ||
| member: value.member, | ||
| direction: match value.direction { | ||
| 0 => OrderDirection::Asc, | ||
| 1 => OrderDirection::Desc, | ||
| _ => unreachable!(), | ||
| }, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Same recommendation: avoid unreachable!() for OrderBy direction.
Like pagination direction, returning an error or default is safer than a panic for unexpected input.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/torii/grpc/src/server/mod.rs (1)
148-175: 🛠️ Refactor suggestionImplementing
From<proto::types::Pagination>andFrom<proto::types::OrderBy>.
- Translating proto fields to local Pagination structure is neat.
- Using
unreachable!()on invalid direction might cause a panic if clients send unrecognized values. Consider returning an error or default direction to avoid runtime panics.- _ => unreachable!(), + _ => { + // Return an error or default + return Self { + cursor: None, + limit: None, + direction: PaginationDirection::Forward, + order_by: vec![], + }; + }
🧹 Nitpick comments (2)
crates/torii/sqlite/src/model.rs (1)
516-523: Combine multiple WHERE clauses.
combine_where_clausesextends an existingbasecondition with any cursor-specific conditions. This is straightforward and helps keep the logic modular.Consider logging or debugging the final
where_clauseif helpful for diagnosing complex queries in production.crates/torii/grpc/src/server/mod.rs (1)
1031-1044: Events retrieval still partially offset-based.While cursor-based logic is used across entities, events appear to keep offset-based logic. That’s presumably an intentional design difference. If you want to unify them, consider future refactoring.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
crates/torii/grpc/proto/world.proto(3 hunks)crates/torii/grpc/src/client.rs(1 hunks)crates/torii/grpc/src/server/mod.rs(28 hunks)crates/torii/sqlite/src/model.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/torii/grpc/src/client.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (23)
crates/torii/sqlite/src/model.rs (9)
1-19: Ohayo sensei! Importing additional types for pagination.The new imports for
HashSet,base64, and pagination types appear logically consistent with the pagination features introduced. Everything looks good here.
298-301: Ohayo sensei! Enhanced function signature forfetch_entities.Adding
pagination: Paginationand returning aResult<Page<SqliteRow>, Error>is a neat approach. This unifies multiple parameters (like limit/offset) into a single struct and returns a more descriptivePage.
341-342: Set safer defaults for limits.Using
pagination.limit.unwrap_or(100)ensures we don’t retrieve an unbounded set of rows. Appending+ 1to detect if there is a “next page” is a neat trick.
344-366: Ohayo sensei! Building theorder_clauselogic with pagination direction.
- The fallback to
format!("{table_name}.event_id DESC")when noorder_byis specified is wise, preventing unpredictable ordering.- When
order_byis present, flipping ASC/DESC based onPaginationDirectionis a smooth solution.Everything looks coherent and correct.
368-384: Neat base64 cursor decoding and validation.
- Successfully handling base64 decoding with error checks is essential.
- Splitting the encoded string with
'/'is neat, as the final join can be re-encoded for the next cursor.All good.
386-420: Chunked schema processing and bridging to pagination.
- Splitting the schemas into chunks ensures queries stay within reasonable join limits (
MAX_JOINS).- Generating a combined “WHERE” condition including cursor checks is orderly.
- Neatly truncating results if they exceed
fetch_limitto detect “has_more.”This approach is carefully structured, sensei!
462-514: Ohayo sensei!build_cursor_conditionsmeticulously handles multiple order-by columns.
- Verifying cursor length with
expected_lenis a solid error check.- Using different comparison operators for forward vs. backward pagination is a clear solution.
- Handling multi-column ordering with the clever “(previous columns = ?) AND (current column operator ?)” logic is a highlight of robust design.
This function strongly underpins correct cursor-based pagination.
525-543: Ohayo sensei! Constructing queries inbuild_query.
- Dynamically assembling
WHEREandHAVINGensures clarity.- Finishing with
GROUP BYandORDER BYlines is consistent with the rest of the code.Everything looks consistent.
545-556:build_cursor_valueselegantly extracts the next cursor from the row.
- Returning only
event_idwhen noorder_byis specified is a consistent fallback.- Collecting multiple columns in
order_byscenario is well-handled.Nice job, sensei!
crates/torii/grpc/proto/world.proto (3)
102-102: Ah, shifting to cursor-based pagination for RetrieveTokens.Replacing offset-based logic with
string cursoris a standard improvement for large-scale paging. Looks good.
148-148: Cursor-based pagination for RetrieveTokenBalances.Continuing the same pattern with
cursor = 5;is consistent with the token retrieval changes.
235-235: Ohayo sensei! Replacingtotal_countwithstring next_cursor.Cursor-based approach in
RetrieveEntitiesResponseis more scalable for big data sets. Just ensure frontends are aware they lose direct total counts.crates/torii/grpc/src/server/mod.rs (11)
15-16: Ohayo sensei! Base64 convenience.Using
BASE64_STANDARD_NO_PADplusEngineis consistent with your decoding logic insqlite::model.
297-304: Refinedentities_allmethod signature.Adding
Paginationand returningPage<proto::types::Entity>is consistent with the overall shift to cursor-based retrieval. This also helps unify logic across the codebase.
317-330:fetch_historical_entities: restricting order-by usage.Rejecting order-by for historical entities is a clear design choice. The error message is explicit, which helps clarify unsupported scenarios for clients.
331-352: Applying cursors to historical entity queries.
- Shifting
event_idcomparisons based on forward/backward direction is a nice approach to reuse the paging logic.- Using base64 decoding for cursors ensures consistency with the rest of the code.
Looks accurate, sensei!
416-426: Pagination usage inquery_by_hashed_keys.Switching from manual limit/offset to the new
paginationobject is consistent. The leftover logic about model filtering looks neatly integrated.
503-529: Ohayo sensei! Constraining queries withkeys_clauseand pagination.
- Converting the old approach of building
where_clausefor the keys into something that merges with the new cursor-based structure is tidy.- The
model_relation_tableusage is consistent with the approach in the rest of the code.Everything merges nicely.
593-645:query_by_memberwith dynamic comparison operator.
- Good job directly flattening the column name into
[model].[member].- The approach of building the final query with
fetch_entitiesandhaving_clauseremains consistent.Works well, sensei!
698-749: Composite clauses + pagination.
- Recursively building composite conditions for AND/OR is flexible.
- Tying them directly to the new pagination logic is consistent with the rest.
The approach to unify all clause types under a single composite is thoroughly done.
927-928: Enforcingpaginationpresence in queries.Raising an error if
paginationis missing ensures the new approach remains mandatory, preventing fallback to offset-based logic.
1393-1408: Ohayo sensei! Introducing cursor-based logic inretrieve_tokens.
- Conditionally appending
WHERE id > ?for the cursor is straightforward.- Reusing the last item’s
idas the next cursor is effective.No immediate concerns, sensei!
835-865: Cursor-based pagination forretrieve_token_balances.
- Similar logic to
retrieve_tokensis consistent.- Tagging “id > ?” ensures ordering clarity by
id.Looks aligned, sensei!
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/torii/grpc/src/server/mod.rs (2)
148-161: Revisiting unreachable!() usage for Pagination direction.
A similar concern was previously flagged about panicking on unexpected input. Consider returning an error or default direction instead of usingunreachable!().
163-175: Same note on unreachable!() usage for OrderBy direction.
Gracefully handling unexpected input is safer than forcing a panic, especially in production services.
🧹 Nitpick comments (3)
crates/torii/grpc/src/server/mod.rs (3)
354-375: Building dynamic SQL queries with order direction.
Ensure columns used in ORDER BY are indexed for performance. This might help with large datasets.
377-384: Potential performance caution on large fetches.
If the result set grows large, consider pagination streaming or chunking.
474-474: Combining fetch with final mapping stage.
Consider using a streaming approach if you expect highly voluminous data. Otherwise, this straightforward design is acceptable.Also applies to: 487-494
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/grpc/src/server/mod.rs(28 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/torii/grpc/src/server/mod.rs (5)
crates/torii/grpc/src/types/mod.rs (20)
from(36-43)from(117-124)from(135-137)from(254-265)from(323-331)from(335-340)from(344-354)from(358-366)from(370-382)from(386-399)from(403-417)from(421-426)from(430-433)from(437-444)from(448-453)from(457-472)value(304-313)value(359-363)value(487-491)value(505-509)crates/torii/sqlite/src/executor/mod.rs (10)
None(302-302)None(815-815)new(168-170)new(243-269)sqlx(312-312)sqlx(447-447)sqlx(533-533)sqlx(562-562)sqlx(627-627)sqlx(714-714)crates/torii/sqlite/src/model.rs (5)
pagination(352-365)pagination(550-554)new(40-74)schema(100-107)fetch_entities(291-461)crates/torii/sqlite/src/cache.rs (2)
models(50-61)model(63-72)crates/torii/sqlite/src/types.rs (1)
from_str(192-201)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (40)
crates/torii/grpc/src/server/mod.rs (40)
15-16: Ohayo sensei! Solid introduction of base64 usage.
This is handy for cursor-based decoding. Ensure that any failures are clearly surfaced to callers.
42-42: Ohayo sensei!
Addinguse tokio::sync::mpsc::Receiver;is a straightforward inclusion for async message handling. No concerns here.
51-53: Ohayo sensei!
These new imports (Pagination,OrderBy, etc.) align well with the cursor-based pagination approach. Great organization!
65-72: Ohayo sensei! Good expansion of the world proto.
Adding these lines for controller & token retrieval requests maintains consistency with the rest of the codebase.
301-304: Ohayo sensei!
Nice introduction ofpagination,no_hashed_keys, andmodelsinentities_allto unify and clarify the query interface.
310-312: Excellent parameter forwarding.
Passing the new arguments consistently intoquery_by_hashed_keysties everything together seamlessly.
317-324: Newfetch_historical_entitiesmethod looks well-structured.
This separation helps handle historical tables distinctly. Good job, sensei!
325-330: Order-by is disallowed for historical entities.
Ohayo sensei, returning an error is straightforward. Just ensure the user gets enough detail if they inadvertently try ordering.
386-418: Mapping fetched rows into Entities.
Implementation here is straightforward. The structure is easy to follow and does not seem to have correctness issues.
423-425: Additional parameters inquery_by_hashed_keys.
Ohayo sensei, this helps unify the approach across the codebase. No issues spotted.
437-442: Bind values from hashed keys.
The format string for hashed keys is correct. Very nice addition!
446-447: Computing model selectors from string references.
This approach is consistent withcompute_selector_from_tag. Good job keeping it DRY.
450-450: Chaining.models(&models)call.
Proper usage of yourmodel_cache. This is consistent with the overall design.
462-472: Redirecting historical queries.
Approach is aligned with the newfetch_historical_entities. Good usage of code reusability.
503-506: Extended signature inquery_by_keys.
Ohayo sensei, the new parameters keep the code parallel toquery_by_hashed_keys.
515-515: Constructingbind_valueswithkeys_pattern.
This ensures the first condition is uniformly handled. Looks good.
520-521: Appending model selectors tobind_values.
Ohayo sensei, the chaining approach is consistent with your dynamic queries.
532-532: Converting model references again.
No issues, sensei. This symmetrical approach fosters maintainability.
541-541: Handlinghaving_clausefor complex queries.
This helps refine the result set effectively. Great job.
548-556: Historical table fallback.
Matches the logic used inquery_by_hashed_keys. Good consistency.
559-579: Mapping results forquery_by_keys.
The final combination of parallel iteration and error-checking is well-structured.
589-592: New parameters inquery_by_member.
All query methods now share a similar pattern for pagination, hashed keys, and models. Great synchronization.
593-594: Selector computation for models.
Ohayo sensei, consistent usage ofcompute_selector_from_tag. Nicely done.
670-673: Flattened column approach forwhere_clause.
Using bracket notation is a neat way to avoid naming collisions.
675-685: Extended fetch pattern inquery_by_member.
Code structure appears consistent and easy to maintain.
688-695: Mapping page results to final Entities.
Parallel iteration looks stable and straightforward.
705-708: Additional parameters forquery_by_composite.
This is consistent with the rest of the refactor.
710-710: Composite clause building.
Smart to encapsulate logic inbuild_composite_clause.
712-730: Combining multiple clauses.
The final result is flexible for complex queries.Also applies to: 735-735
741-748: Returning a page of Entities.
The usage of next_cursor stands out as user-friendly for navigation.
927-928: Extracting pagination.
Pulling fromquery.paginationensures consistent error handling.
930-1001: Clause matching for retrieving entities.
Including hashed_keys, keys, member, and composite logic covers all scenarios thoroughly.
1003-1006: FinalizingRetrieveEntitiesResponse.
Concise and user-friendly, sensei. Excellent.
1009-1009:retrieve_events_internalsignature.
Good naming—it aligns with the rest of the retrieval methods.
1393-1393: ParsingRetrieveTokensRequest.
Ohayo sensei, the approach is consistent with cursor-based logic used elsewhere.
1311-1329: New public gRPC API:retrieve_entities.
The forward call to internal logic is well-structured, with robust error mapping.
1331-1353:retrieve_event_messagesgRPC method.
Follows the same pattern. Confident usage of shared retrieval logic fosters maintainability.
1355-1370:retrieve_eventsgRPC method.
Matches usage inretrieve_events_internal. Clear flow with minimal duplication.
517-517:❓ Verification inconclusive
Regex usage with
REGEXP.
Ensure the SQL environment supports this reliably; otherwise, watch for potential injection vectors.
🏁 Script executed:
#!/bin/bash # Checking if the REGEXP function is available in current sqlite builds sqlite3 --version || trueLength of output: 124
Ensure sqlite3 REGEXP Support and Injection Safety
Ohayo sensei, it looks like our automated check didn’t yield the expected sqlite version output—the sqlite3 command wasn’t available in the current environment. Could you please manually verify that the SQL environment reliably supports the REGEXP function? Also, please double-check that interpolating the table name in the query (as seen on line 517 of
crates/torii/grpc/src/server/mod.rs) is safe and does not inadvertently expose injection vulnerabilities.
331-352:❓ Verification inconclusive
Cursor-based filtering on event_id.
Ohayo sensei, confirm thatevent_idis stored as a numeric type or a consistently padded string in the DB; otherwise, string comparisons may incorrectly order events.
🏁 Script executed:
#!/bin/bash # Confirm data type for event_id in the DB schema sqlite3 :memory: '.schema entities_historical' || true sqlite3 :memory: '.schema event_messages_historical' || trueLength of output: 313
Ohayo sensei – Manual Schema Verification Required for event_id
The previous sqlite3 check couldn’t run, so we couldn’t automatically verify that the DB schema defines the
event_idcolumn as a numeric type (or a properly padded string). Please run the following shell script to search for the table definitions (in migrations or schema files) forentities_historicalandevent_messages_historical. Verify that in their schema definitions theevent_idcolumn is defined as either a numeric type (e.g., INTEGER) or, if it’s text, is consistently formatted for proper ordering.Once you’ve confirmed that the
event_idcolumn meets these criteria, we can be confident that the cursor-based filtering orders events correctly.
🏁 Script executed:
#!/bin/bash rg -C3 "CREATE TABLE.*(entities_historical|event_messages_historical)" .Length of output: 1681
Ohayo sensei – Please Verify DB Event ID Format
Our migration outputs indicate that the key column for historical events (named "id" in the migrations for both
event_messages_historicalandentities_historical) is defined asTEXT NOT NULL. The code currently applies cursor-based filtering using lexicographic comparisons (i.e.,{table}.event_id >= ?or<= ?), which will work correctly only if the text values are consistently formatted (e.g., via zero-padding) or if they represent numeric values in a string-safe format.
- Confirm that the
event_idin the code corresponds to theidcolumn from the migrations.- Ensure that the stored text values are either padded consistently or represent numeric strings so that lexicographic ordering behaves as expected.
Please double-check the schema details and the business logic surrounding ID generation to avoid any potential ordering issues.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3147 +/- ##
==========================================
- Coverage 55.60% 55.51% -0.10%
==========================================
Files 443 443
Lines 62957 62734 -223
==========================================
- Hits 35008 34827 -181
+ Misses 27949 27907 -42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/torii/grpc/proto/types.proto (1)
195-198: Ohayo sensei: New PaginationDirection EnumThe introduction of the
PaginationDirectionenum withFORWARDandBACKWARDis a solid step for explicit cursor navigation. Consider adding inline documentation or comments to assist future developers with its intended use.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
crates/torii/grpc/proto/types.proto(2 hunks)crates/torii/grpc/proto/world.proto(4 hunks)crates/torii/grpc/src/server/mod.rs(28 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/grpc/proto/world.proto
- crates/torii/grpc/src/server/mod.rs
🔇 Additional comments (4)
crates/torii/grpc/proto/types.proto (4)
74-77: Ohayo sensei: Enhanced Query Message FieldsThe
Querymessage now cleanly renames and consolidates its fields—no_hashed_keysreplaces the ambiguousdont_include_hashed_keys, andmodelsis a clearer representation. The addition of thePaginationfield and the newhistoricalflag streamline pagination support. Please verify that all services and clients consuming this message are updated accordingly.
83-83: Ohayo sensei: EventQuery Cursor Field UpdateReplacing the numeric offset with a
string cursorin theEventQuerymessage aligns well with cursor-based pagination. Ensure that the query logic which interprets this field is updated and thoroughly tested.
193-194: Ohayo sensei: Controller Message Formatting CheckThe tilde markers on the closing brace of the
Controllermessage suggest a formatting change. There appears to be no semantic modification besides this; please confirm that this was intentional and does not affect message serialization.
200-204: Ohayo sensei: New Pagination Message for Cursor-Based PaginationThe
Paginationmessage now encapsulatescursor,limit,direction, andorder_byfields, providing a cohesive structure for pagination. Verify that all downstream consumers can correctly interpret these fields and that any legacy code is properly migrated.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/torii/grpc/src/server/mod.rs (2)
148-161: Ohayo sensei, consider handling unknown direction more gracefully.
Usingunreachable!()for an unexpected pagination direction may cause unexpected panics if the input is invalid. A safer approach is to return an error or default direction.
163-175: Ohayo sensei, the same unreachable! logic forOrderBydirection might need a fallback.
Similar to pagination direction, relying onunreachable!()could lead to panics when encountering invalid input. Consider returning an error or defaulting toAsc.
🧹 Nitpick comments (1)
crates/torii/grpc/src/server/mod.rs (1)
431-431: Ohayo sensei, consider using a SQLINclause for potentially faster lookups.
Stringing multiple conditions withORis valid but might be less optimal than a singleINexpression when dealing with many hashed keys.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/grpc/src/server/mod.rs(29 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (15)
crates/torii/grpc/src/server/mod.rs (15)
15-16: Ohayo sensei, base64 usage looks good!
These imports seem valid and consistent with your decoding/encoding approach. Ensure that using theNO_PADvariant is intentional, as some clients or consumers may expect standard padding.
42-42: Ohayo sensei, additional import recognized.
ThisReceiverimport fromtokio::sync::mpscis aligned with existing subscription logic. No concerns here.
52-53: Ohayo sensei, newly introduced type imports appear correct.
These imported types (e.g.Page,Pagination) are essential for cursor-based pagination and token management flows. Nicely organized.
65-72: Ohayo sensei, these added imports for proto messages are in order.
They match the updated pagination and token retrieval features. Great job keeping them consistent.
301-304: Ohayo sensei, expanded function signature forentities_allis consistent.
Addingpagination,no_hashed_keys, andmodelsparameters aligns this function with your new pagination logic. Looks coherent.
310-313: Ohayo sensei, passing newly introduced arguments is seamless.
Forwardingpagination,no_hashed_keys, andmodelstoquery_by_hashed_keysensures consistency.
317-376: Ohayo sensei, watch out for potential duplicate records when using>=or<=in pagination.
While usingevent_id >= ?orevent_id <= ?for the cursor can simplify logic, it may also cause repeated records between pages if the sameevent_idspans multiple rows. Consider strict comparisons (>/<) or additional tie-breakers to avoid duplicates.
416-426: Ohayo sensei, new pagination parameters inquery_by_hashed_keyslook well-integrated.
The updated method signature cleanly brings in pagination, hashed key usage, and model constraints. Nicely done.
689-695: Ohayo sensei, parallel entity mapping is looking efficient.
Usingpar_iter()withrayonis a solid approach to deserialize rows concurrently. Ensure no shared-state mutation occurs.
927-928: Ohayo sensei, requiring a pagination object for all queries is a great approach.
This enforces a consistent interface and clarifies the query flow.
930-960: Ohayo sensei, new logic branches for different clause types elegantly incorporate pagination.
All clause types (hashed keys, keys, member, composite) funnel nicely into the pagination flow. This uniform handling helps maintain consistent query behavior.Also applies to: 981-996, 1003-1006
1013-1014: Ohayo sensei, validate the ordering logic when applying cursor-based pagination to events.
Currently, you limit results byORDER BY id DESC, then applyWHERE id >= ?for the cursor. This might include items out of order if cursor logic expects ascending sequences. Consider inverting the comparison or direction if retrieving older events first.Also applies to: 1023-1034, 1036-1056, 1068-1068
1113-1113: Ohayo sensei,map_row_to_eventfunction is straightforward.
Decoding keys and data from base64 properly withFeltusage looks correct. Nicely done.
1326-1344: Ohayo sensei, new retrieve methods gracefully leverage pagination.
Methods likeretrieve_entities,retrieve_event_messages, andretrieve_eventsshow consistent usage of your new approach. Great for code uniformity.Also applies to: 1346-1368, 1370-1383
1406-1425: Ohayo sensei, token retrieval with optional limit and cursor is implemented cleanly.
Be sure to confirm that partial retrieval and next-cursor generation handle edge cases (e.g., small sets) correctly in tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
crates/torii/grpc/src/server/mod.rs (1)
666-673: 🛠️ Refactor suggestionCentralize
prepare_comparisonlogic for DRY compliance.
Ohayo sensei! This member comparison handling reappears in multiple spots (e.g., lines 1231–1267). Consolidating into a shared utility would improve readability and reduce duplication.
♻️ Duplicate comments (2)
crates/torii/grpc/src/server/mod.rs (2)
153-156: Avoid usingunreachable!()for unknown pagination direction.
Ohayo sensei! Returning an explicit error is safer than panicking if the direction is invalid. Replacing this branch with a fallback or error path would improve robustness.
169-172: Handle unknown order direction gracefully.
Ohayo sensei! Same recommendation: use a fallback or return an error for unknown directions instead ofunreachable!().
🧹 Nitpick comments (2)
crates/torii/grpc/src/server/mod.rs (2)
317-415: Consider refactoring thefetch_historical_entitieslogic.
Ohayo sensei! This block duplicates pagination and query-building logic that appears elsewhere. Extracting shared code into a helper function would streamline maintenance and reduce potential inconsistencies.
1333-1344: Consider adding unit tests for pagination paths.
Ohayo sensei! The newly integrated pagination parameters are only partially tested. Strengthening test coverage for edge cases (e.g., empty cursors, large limits) will ensure correctness.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
crates/torii/client/src/client/mod.rs(3 hunks)crates/torii/grpc/src/server/mod.rs(29 hunks)crates/torii/grpc/src/types/mod.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/torii/grpc/src/types/mod.rs
- crates/torii/client/src/client/mod.rs
🔇 Additional comments (1)
crates/torii/grpc/src/server/mod.rs (1)
1052-1068: Double-check pagination and sorting for events.
Ohayo sensei! The next cursor is derived from the first column (id), but the query also sorts byid DESC. Please verify that the cursor comparison logic matches the sort order to avoid missing or repeating events.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/torii/grpc/src/server/mod.rs (2)
154-156: Handle unknown pagination directions instead of panicking.
This replicates past feedback about avoidingunreachable!()when an unexpected direction arises.
169-171: Handle unknown order directions instead of panicking.
Same as before: returning an error or a default direction is safer than usingunreachable!().
🧹 Nitpick comments (2)
crates/torii/grpc/src/server/mod.rs (2)
317-353: Ohayo sensei! Confirm inclusive cursor usage.
Using>=or<=with the cursor can introduce duplicates when paging forward/backward. You may want strict comparisons (>/<) or to store the last-seen ID offset.Are you certain about including the current
event_idin subsequent pages?
835-866: Ohayo sensei!
Inretrieve_token_balances, using base64 cursors is consistent with your other pagination features. Please double-check thatidhas an index to handle large data efficiently.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/grpc/src/server/mod.rs(27 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (12)
crates/torii/grpc/src/server/mod.rs (12)
15-16: Ohayo sensei! Base64 integration looks neat.
All usages ofBASE64_STANDARD_NO_PADfor cursor encoding/decoding appear consistent, and error handling viaError::InvalidCursoris in place.
42-42: Ohayo sensei!
Importingtokio::sync::mpsc::Receiverfor concurrency is aligned with the usage ofblock_rxtasks.
50-53: Ohayo sensei!
These newly imported types fromtorii_sqlite::types(e.g.Pagination,Page,OrderBy) cleanly support the new paging logic.
65-72: Ohayo sensei!
Expanding proto imports (e.g.,RetrieveTokenBalancesRequest,RetrieveTokensRequest) aligns with your new services.
295-313: Ohayo sensei! The updatedentities_allsignature looks good.
Introducingpagination,no_hashed_keys, andmodelsis clean. Delegating toquery_by_hashed_keysis straightforward and consistent.
423-426: Ohayo sensei!
New parameters inquery_by_hashed_keysare consistent with the other query methods. Good step in unifying the pagination approach.
436-444: Ohayo sensei! Validate slice lengths.
Felt::from_bytes_be_slice(key)assumes 32-byte input. Consider gracefully handling mismatches to prevent potential panics.
462-472: Ohayo sensei!
Redirecting tofetch_historical_entitiesfor_historicaltables is a clean approach. The boundary between current vs. historical queries is well-defined.
589-592: Ohayo sensei!
The new parameters inquery_by_membermirror the consistent pattern of passingpagination,no_hashed_keys, andmodels. This retains a uniform interface across queries.
1013-1014: Ohayo sensei!
Incrementing the limit by one for pagination is a standard approach to detect the presence of further results. Looks good.
1119-1123: Ohayo sensei!
Mapping rows to events withmap_row_to_eventis concise. Great job reusingprocess_event_fieldfor keys/data decoding.
1412-1412: Ohayo sensei!
New parameterslimitandcursorinRetrieveTokensRequestconfirm a uniform pagination strategy across tokens.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/torii/grpc/src/server/mod.rs (2)
148-161: Ohayo sensei, unreachable!() usage forPaginationDirection
This was flagged in a past review, recommending returning an error or default direction rather than panicking on unknown input.
163-175: Ohayo sensei, unreachable!() usage forOrderBydirection
Same concern as before: consider gracefully handling unexpected input to avoid panics.
🧹 Nitpick comments (1)
crates/torii/grpc/src/server/mod.rs (1)
317-415: Ohayo sensei, historical entities fetching
This new function thoroughly handles cursor-based pagination for historical data, including proper base64 decoding and ordering byevent_id. It looks cohesive.Consider adding test coverage to verify boundary conditions (e.g., empty cursor vs. valid cursor, minimal vs. large limit).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
crates/torii/grpc/src/server/mod.rs(27 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (11)
crates/torii/grpc/src/server/mod.rs (11)
15-16: Ohayo sensei, new base64 engine import
No immediate concerns. The usage ofBASE64_STANDARD_NO_PADandEnginelooks straightforward.
42-42: Ohayo sensei, additionalReceiverimport
No issues spotted. This import aligns with the subscription flow established below.
301-304: Ohayo sensei, extended function signature for pagination
Bringingpagination,no_hashed_keys, andmodelsintoentities_allenhances flexibility. No correctness issues found.Also applies to: 310-312
423-425: Ohayo sensei, updated signature & hashed keys handling
Addingpagination,no_hashed_keys, andmodelsplus joining hashed keys with "OR" seems correct for broader filtering.Also applies to: 431-431, 441-442
463-471: Ohayo sensei, branching logic for historical vs. active entities
The condition to callfetch_historical_entitiesfor tables ending in_historicalis intuitive, and the fallback tofetch_entitiesis clearly defined.Also applies to: 474-495
498-507: Ohayo sensei, consistent pagination approach inquery_by_keys
The expansions forpagination,no_hashed_keys, andmodelsare integrated throughout the function’s flow. This maintains a uniform structure across queries.Also applies to: 515-521, 531-557, 561-580
589-592: Ohayo sensei, new arguments inquery_by_member
The approach to incorporatepagination,no_hashed_keys, andmodelsfor filtered lookups appears correct. Data mapping is consistent with the rest of the codebase.Also applies to: 593-645
699-708: Ohayo sensei, composite clause logic
Combining multiple clauses withORorANDis a neat approach. The recursive handling for nested composites is well-structured.Also applies to: 709-749
927-929: Ohayo sensei,retrieve_entities()andretrieve_events()
Introducing pagination logic and handling various clause types systematically is a solid upgrade. It simplifies client-side integration.Also applies to: 930-930, 936-939, 953-953, 957-959, 999-999, 1003-1006, 1013-1075
1119-1123: Ohayo sensei,map_row_to_eventchanges
Decoding transaction hashes with fallback errors ensures data correctness. Implementation is consistent and succinct.
1332-1350: Ohayo sensei, new gRPC endpoints and subscription flows
These expansions round out the pagination features across the service. The changes appear to align well with the newPaginationstruct and subscription patterns.Also applies to: 1352-1374, 1376-1389, 1412-1412, 1596-1599, 1611-1618
| let where_clause = format!( | ||
| "[{}].[{}] {comparison_operator} {value}", | ||
| member_clause.model, member_clause.member | ||
| ); |
There was a problem hiding this comment.
Ohayo sensei, potential SQL injection risk on dynamic column formatting
String interpolation like format!("[{}].[{}]", ...) might allow malicious input if user-controlled. Consider validating or whitelisting column names to avoid injection.
Here’s a possible safeguard:
- let where_clause = format!("[{}].[{}] {comparison_operator} {value}", member_clause.model, member_clause.member);
+ // Validate or whitelist member_clause.model and member_clause.member before using them.
+ let validated_model = sanitize_or_whitelist_model(&member_clause.model)?;
+ let validated_member = sanitize_or_whitelist_member(&member_clause.member)?;
+ let where_clause = format!("[{}].[{}] {comparison_operator} {value}", validated_model, validated_member);Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Page,Pagination, andPaginationDirection, improving the handling of paginated data.Bug Fixes
Changes
historicalparameter from entity retrieval requests, simplifying the request structure.offsetparameter from various request methods, streamlining the API interactions.