Rework graphql pagination #113
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Move the base.rs file from the integration tests of the game-api package to a proper crate, used as dev-dependencies by game-api and newly graphql-api. * Remove the "test" feature, check directly for cfg(test) in game-api package and integration tests. Replaced by "mock" for mock database related stuff in the records-lib package. * Bump version of mkenv, and adapt the definition of configuration using the new design. * Define a configuration for the graphql-api package, used to host the default and max limits of the pagination accross every GraphQL node. They were previously defined with hard-coded constants in the code. * The graphql-api configuration also hosts the secret key used to sign the cursors for paginating. Signing the cursors is useful to simplify the check of a previous/next page, since if the decoding of a cursor bound to an input "after" or "before", it means the user got it from our own API, so it's safe to consider that there is a previous/next page, depending on the situation. * Update the records_lib::ranks module, to remove the transaction stuff of the get_rank function, which was useless. There is no need to add/remove the time of the player in the ZSET before/after getting the rank. This implies the removal of the `RankingSession` type, and the `get_rank_impl` and `get_rank_in_session` function. The body of the `get_rank` function is now very short and clear. This also implies that the argument for the redis connection is now a simple `&mut RedisConnection`, like before, and not a `&RedisPool`. * WIP: for now, only `QueryRoot.recordsConnection` pagination is functional. Other GraphQL paginable nodes should be updated in the next commit. * WIP: added tests for the `QueryRoot.recordsConnection` GraphQL node. This is a draft, and the tests should be refactored a bit, since there is some duplicated fragments between them. * Add tests for encoding/decoding cursors containing a date or a date with a time. * Pass clippy everywhere. Refs: #111.
* Add tests for all required GraphQL nodes. * Make cursors to be also based on record IDs or player IDs in case of duplicate record date, time or score. * Make GraphQL nodes `QueryRoot.playersConnection` and `QueryRoot.mapsConnection` to be ordered on the rank by default. In the future, they will have a similar behavior as the `Map.recordsConnection` node, meaning be ordered by rank by default, and could be sorted by names instead. * To allow filtering while ordering the above nodes by rank, SQL columns are added in a new migration to store the score of each player/map, and they're used as indexes for pagination. In the future, this could also be used for filtering. This change implies a change in the socc package, to save the score of each player/map to the SQL database instead of only in Redis. The Redis server is still needed to quickly fetch the rank of a player/map, like what is done for records. Refs: #111.
* Make similar pattern to manage the sort input of the `QueryRoot.maps` and `QueryRoot.players` GraphQL nodes, like the `Map.recordsConnection` node (manage different kind of cursors). * Also escape the text and data when encoding/decoding the cursors, by replacing ':' with '\:' and vice versa. Refs: #111.
This refactor mainly affects GraphQL node having multiple pagination options, such as `QueryRoot.players`, `QueryRoot.maps`, and `Map.recordsConnection`, because they can be paginated/sorted using various cursors. For these nodes, the construction of the cursor encoder is made earlier in the function, at the same time as during construction of the type of cursor to manage the pagination. The type of cursor to manage the pagination is chosen based on the `sort` input for now. This will probably be changed later, since the `sort` input could be based on a different cursor than the one used for pagination. To take the above remark into accout, in the function of these same nodes, the query building is based on the type of cursor we manage, instead of only the `sort` field. Refs: #111.
* Replace split mechanism with JSON serialization/deserialization for cursor encoding/decoding. * This implies a transform of the cursor structs from tuple structs to named structs. * Make the serialization/deserialization of datetimes to use the timestamp instead of the string ISO format. * Keep the prefix mechanism, so that a cursor type is quickly recognizable from it. This could be useful in the future, to know which cursor type to use based on the user input, by testing all the cursor types and taking the first that matches the corresponding prefix. * Add more tests with also text and score cursor types. Refs: #111.
* Instead of having only `ID`s as input for cursor types, use the concrete cursor types so that the `query_with` function from the async-graphql crate parses and inject them directly. Refs: #111.
* Re-create the `sea_orm::Cursor` struct with related traits and impls to fix a problem related to the condition made for paginating the result in the `WHERE` clause of the generated SQL statement. * Use the new cursor version in every GraphQL node that use cursor pagination. Refs: #111.
This allows to precise in front of every error type in the `fmt::Display` implementation the "cursor decode error" mention. Refs: #111.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR performs a comprehensive refactoring of the GraphQL API's cursor pagination system. The changes introduce HMAC-signed cursors for security, add score fields to players and maps tables for improved performance, simplify the ranking system by removing the RankingSession abstraction, and migrate to a new version of the mkenv configuration library. A new test infrastructure crate is introduced to support comprehensive integration testing.
Key Changes:
- Introduced HMAC-based cursor signing for security using secret keys
- Added
scorefields toplayersandmapstables with corresponding migration - Replaced
RankingSessionwith directget_rankcalls for simpler rank retrieval - Migrated from
mkenvv1 to v2 with new configuration API - Created new
test-envcrate for test database management - Added comprehensive integration tests for pagination scenarios
Reviewed changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
crates/test-env/ |
New crate providing test environment setup with database isolation |
crates/graphql-api/src/cursors.rs |
Complete rewrite with HMAC signing, JSON serialization, and improved type safety |
crates/graphql-api/src/config.rs |
New config module for cursor limits and secret key management |
crates/graphql-api/src/utils/ |
New pagination utilities for cursor handling and filtering |
crates/records_lib/src/ranks.rs |
Simplified by removing RankingSession and complex transaction logic |
crates/records_lib/src/env.rs |
Migrated to mkenv v2 configuration system |
crates/migration/ |
Added migration for score fields on players and maps |
crates/socc/src/player_ranking.rs |
Updated to persist scores to SQL and Redis |
crates/graphql-api/src/objects/root.rs |
Refactored pagination logic with new cursor system |
crates/graphql-api/src/tests/ |
New comprehensive integration tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This follows the upgrade of the mkenv package, which requires version 1.92.0 of rustc. Refs: #111.
* Replace `cfg(test_force_db_deletion)` use in test-env package with argument `--force-drop-db`. * Upgrade dependencies. * Remove use of "test" feature flag in CI. * Remove clippy check in CI without any feature (`--no-default-features`), and add at least one feature for DB selection (`mysql` for now). Refs: #111.
Refs: #111.
* Add missing env var in CI. * Add clippy in release mode in CI. * Fix compilation issues in release mode. * Fix import not done in release mode. Refs: #111.
Contributor
There was a problem hiding this comment.
Copilot reviewed 70 out of 70 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors the graphql-api crate to rework the cursor pagination of GraphQL node that use it, such as:
QueryRoot.mapsQueryRoot.playersQueryRoot.recordsConnectionPlayer.recordsConnectionMap.recordsConnection