Refactor getSequences #618
Conversation
WalkthroughAn abstract Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant Adapter
participant SQL
Client->>Database: createDocuments(collection, documents)
Database->>Adapter: createDocuments(collection, documents)
Adapter->>SQL: createDocuments(collection, documents)
SQL-->>Adapter: [inserted documents]
Adapter-->>Database: [inserted documents]
Database->>Adapter: getSequences(collection, documents)
Adapter->>SQL: getSequences(collection, documents)
SQL-->>Adapter: [documents with sequences]
Adapter-->>Database: [documents with sequences]
Database-->>Client: [documents with sequences]
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ 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: 7
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Database/Adapter.php(1 hunks)src/Database/Adapter/MariaDB.php(1 hunks)src/Database/Adapter/Pool.php(1 hunks)src/Database/Adapter/SQL.php(1 hunks)src/Database/Database.php(2 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)
🧰 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.
🧬 Code Graph Analysis (2)
src/Database/Adapter.php (2)
src/Database/Adapter/Pool.php (1)
getSequences(493-496)src/Database/Adapter/SQL.php (1)
getSequences(720-771)
src/Database/Adapter/Pool.php (3)
src/Database/Adapter.php (1)
getSequences(736-736)src/Database/Adapter/SQL.php (1)
getSequences(720-771)src/Database/Mirror.php (1)
delegate(88-103)
🪛 GitHub Actions: CodeQL
src/Database/Adapter.php
[error] 736-736: PHPStan: Method Utopia\Database\Adapter::getSequences() has parameter $documents with no value type specified in iterable type array.
[error] 736-736: PHPStan: Method Utopia\Database\Adapter::getSequences() return type has no value type specified in iterable type array.
src/Database/Adapter/Pool.php
[error] 493-493: PHPStan: Method Utopia\Database\Adapter\Pool::getSequences() has parameter $documents with no value type specified in iterable type array.
[error] 493-493: PHPStan: Method Utopia\Database\Adapter\Pool::getSequences() return type has no value type specified in iterable type array.
src/Database/Adapter/SQL.php
[error] 770-770: PHPStan: Method Utopia\Database\Adapter\SQL::getSequences() should return array but returns array<Utopia\Database\Document>.
🪛 GitHub Actions: Linter
tests/e2e/Adapter/Scopes/DocumentTests.php
[error] 1-1: PSR 12 linting failed: statement_indentation style issue detected.
src/Database/Adapter/SQL.php
[error] 1-1: PSR 12 linting failed: statement_indentation style issue detected.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (5)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
449-449: Good validation of sequence data.This assertion correctly verifies that documents returned from upsert operations contain the expected
$sequencekey, which aligns with the refactored sequence handling mentioned in the summary.src/Database/Adapter/MariaDB.php (1)
1188-1188: Sequence Handling VerifiedI’ve confirmed that:
getSequencesis declared in src/Database/Adapter.php and implemented in src/Database/Adapter/SQL.php (with Pool adapter delegating to it).- src/Database/Database.php invokes
$this->adapter->getSequences(...)immediately after each batch of create/update calls.- All sequence-fetching logic has been successfully moved out of the MariaDB adapter and centralized at the Database class level.
This refactoring cleanly separates database-specific operations from business logic. LGTM! 🚀
src/Database/Database.php (2)
3754-3754: LGTM! Proper sequence update placement.The addition of
getSequencescall is well-positioned after the batch creation and before relationship resolution and decoding. This ensures documents have updated sequence data before further processing.
5071-5071: LGTM! Consistent sequence handling.Good consistency with the
createDocumentsmethod - both batch operations now properly update sequence data using the same pattern and placement.src/Database/Adapter/SQL.php (1)
736-736: Fix tenant binding key concatenation.The tenant binding key uses incorrect string concatenation syntax. It should use curly braces for proper interpolation.
Apply this diff to fix the tenant binding key:
- $binds[':_tenant_'.$i] = $document->getTenant(); + $binds[":_tenant_{$i}"] = $document->getTenant();Likely an incorrect or invalid review comment.
Summary by CodeRabbit
New Features
Bug Fixes
Tests