Fix cursor #615
Fix cursor #615
Conversation
|
""" WalkthroughThe changes refactor the cursor-based pagination and ordering logic in the MariaDB and Postgres database adapters, simplifying and unifying the construction of pagination WHERE clauses. The main Database class is updated to set a default cursor direction, ensure unique ordering by appending a sequence attribute if necessary, and validate cursor completeness. Additionally, new tests verify multi-attribute ordering and cursor pagination behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant Adapter
Client->>Database: find(collection, queries, forPermission)
Database->>Database: Set default cursor direction if not provided
Database->>Database: Ensure unique order attribute (append $sequence if missing)
Database->>Database: Validate cursor completeness (throw OrderException if missing)
Database->>Adapter: find(..., cursorDirection, ...)
Adapter->>Adapter: Build uniform pagination WHERE clause based on order attributes and cursor
Adapter-->>Database: Return results
Database-->>Client: Return results
Poem
📜 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 (11)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
src/Database/Adapter/MariaDB.php(2 hunks)src/Database/Adapter/Postgres.php(1 hunks)src/Database/Database.php(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Database/Adapter/MariaDB.php (6)
src/Database/Adapter/SQL.php (2)
getInternalKeyForAttribute(1712-1724)getSQLOperator(1432-1458)src/Database/Adapter.php (2)
filter(1105-1114)quote(447-447)src/Database/Database.php (1)
Database(35-6790)src/Database/Adapter/Postgres.php (1)
quote(2007-2010)src/Database/Adapter/Pool.php (1)
quote(118-121)src/Database/Query.php (1)
Query(8-730)
src/Database/Database.php (2)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Document.php (1)
getAttribute(212-219)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (7)
src/Database/Database.php (5)
16-16: LGTM: Necessary import for cursor validation.The OrderException import is correctly added to support the new cursor validation logic introduced in the find method.
6018-6019: LGTM: Appropriate default cursor direction.Setting
Database::CURSOR_AFTERas the default cursor direction when none is provided ensures consistent pagination behavior and centralizes the default logic in the main Database class rather than leaving it to individual adapters.
6020-6029: LGTM: Essential unique ordering enforcement.This logic ensures that pagination results have deterministic ordering by requiring either
$idor$sequencein the order attributes. Appending$sequencewhen no unique attribute is present is a solid approach that guarantees consistent pagination behavior across different queries.
6031-6040: LGTM: Robust cursor validation.The validation ensures that the cursor document contains non-null values for all order attributes, which is essential for accurate cursor-based pagination. The use of
OrderExceptionwith both a descriptive message and the specific attribute name provides good debugging information.The logic correctly handles the case where
$sequencemight be dynamically added to the order attributes, ensuring the cursor validation is comprehensive.
6108-6108: LGTM: Explicit cursor direction parameter.Passing the cursor direction explicitly to the adapter method call ensures that the centralized logic in the Database class takes precedence over any adapter-specific defaults, which aligns with the overall refactoring goals mentioned in the summary.
src/Database/Adapter/MariaDB.php (1)
1519-1568: Well-implemented cursor pagination logic.The refactored cursor pagination implementation correctly:
- Handles multiple order attributes with proper equality conditions for preceding attributes
- Reverses sort direction appropriately for
CURSOR_BEFORE- Builds the WHERE clause with proper OR conditions for cursor-based pagination
- Uses parameterized queries with unique bind names to prevent conflicts
This unified approach simplifies the logic and makes it more maintainable.
src/Database/Adapter/Postgres.php (1)
1401-1450: Consistent cursor pagination implementation across adapters.The Postgres adapter implementation correctly mirrors the MariaDB approach, ensuring consistent behavior across different database backends. The cursor pagination logic properly handles:
- Multiple order attributes with appropriate conditions
- Direction reversal for
CURSOR_BEFORE- Proper parameter binding with unique names
- Postgres-specific quoting
This unified approach improves code maintainability and reduces the likelihood of adapter-specific bugs.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Adapter/MariaDB.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Database/Adapter/MariaDB.php
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#613
File: src/Database/Adapter/Postgres.php:1254-1319
Timestamp: 2025-07-01T11:31:37.438Z
Learning: In PostgreSQL adapter methods like getUpsertStatement, complexity for database-specific SQL generation is acceptable when the main business logic is properly separated in the parent SQL adapter class, following the adapter pattern where each database adapter handles its own SQL syntax requirements.
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Unit Test
🔇 Additional comments (1)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
1904-1951: Well-structured test for multi-attribute ordering and cursor pagination.This test effectively validates:
- Multi-attribute ordering (year ascending, then price ascending) works correctly
- The expected sort order of movies is maintained
- Cursor pagination (
cursorAfter) functions properly with multi-attribute ordering- The correct subset of documents is returned after a cursor position
The test coverage aligns well with the cursor pagination refactoring mentioned in the PR objectives and provides good validation for the enhanced cursor logic.
Summary by CodeRabbit
Refactor
Bug Fixes
Chores
Tests