$sequence cast empty string issue #617
Conversation
WalkthroughThe changes remove special-case logic for handling the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PostgresAdapter
participant Database
Client->>PostgresAdapter: find(query)
PostgresAdapter->>Database: Prepare SQL with parameters (all bound via getPDOType())
Database-->>PostgresAdapter: Return results
PostgresAdapter-->>Client: Return documents
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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
🔭 Outside diff range comments (2)
src/Database/Database.php (2)
678-690: Remove commented-out code or document its purpose.This commented-out code block appears to be a PostgreSQL-specific workaround for empty string to integer conversion issues with the
$sequenceattribute. However, having large blocks of commented code in production reduces readability and can be confusing.Based on the PR objectives, this issue is being addressed by updating the default sequence value to
'0'in the Document class and removing special-case handling in the Postgres adapter. Since this approach has been chosen, this commented code should be removed entirely.If this code needs to be preserved for reference or future use, consider:
- Moving it to a code comment with proper documentation explaining why it was considered but not implemented
- Creating a separate issue to track this as a potential alternative approach
-// if($query->getAttribute() === '$sequence'){ -// /** -// * Hack for Postgres, since bindParam does not convert '' on int attribute -// */ -// $values = $query->getValues(); -// foreach ($values as $valueIndex => $value) { -// if($value === ''){ -// $values[$valueIndex] = '0'; -// } -// } -// $query->setValues($values); -// $queries[$index] = $query; -// }
676-677: Fix indentation to resolve linting error.The pipeline failure indicates a PSR-12 statement indentation issue. The
foreachloop appears to have incorrect indentation.- foreach ($queries as $index => $query) { + foreach ($queries as $index => $query) { if ($query->getAttribute() === $attribute->getId()) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Database/Adapter/Postgres.php(2 hunks)src/Database/Database.php(1 hunks)src/Database/Document.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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.
src/Database/Adapter/Postgres.php (1)
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 (3)
src/Database/Adapter/Postgres.php (2)
src/Database/Adapter/MariaDB.php (1)
getPDOType(1957-1965)src/Database/Adapter/SQL.php (1)
getPDOType(1544-1544)
tests/e2e/Adapter/Scopes/DocumentTests.php (4)
src/Database/Document.php (3)
Document(12-465)getSequence(67-75)find(299-317)src/Database/Adapter/Postgres.php (1)
find(1390-1552)src/Database/Database.php (1)
find(5971-6132)src/Database/Query.php (2)
Query(8-730)equal(355-358)
src/Database/Database.php (2)
src/Database/Document.php (2)
getAttribute(217-224)getId(59-62)src/Database/Query.php (1)
getAttribute(120-123)
🪛 GitHub Actions: Linter
src/Database/Database.php
[error] 1-1: PHP CS Fixer (Laravel Pint) PSR-12 style check failed: statement_indentation issue.
⏰ 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 (3)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
1782-1797: Verify necessity and document limitations of the PostgreSQL “0” sequence workaroundThis test correctly exercises the temporary hack in
convertQueriesthat turns an emptygetSequence()("") into"0"to avoid Postgres bind-parameter errors. Before landing this, please confirm and document:• Semantic correctness
– Does an empty sequence ("") carry a different business meaning than"0"?
• Query behavior
– Could real documents with sequence"0"now collide or return unintended results?
• Cross-database impact
– Other adapters (SQLite, MariaDB) skip empty sequences (if (!empty())), while Postgres will now bind"0". Ensure this change doesn’t break those adapters.Key locations to review:
• src/Database/Document.php →getSequence()
• src/Database/Database.php →convertQueries()(sequence filter handling)
• src/Database/Adapter/Postgres.php → binding logic around:_sequence
• src/Database/Adapter/SQLite.php & MariaDB.php → how empty sequences are filtered outAs a more robust long-term approach, consider skipping or short-circuiting empty-sequence filters instead of converting to
"0". For example, inconvertQueries():if ($field === '$sequence' && (string)$value === '') { // Skip sequence‐based filter to avoid binding errors continue; }src/Database/Adapter/Postgres.php (2)
1507-1507: LGTM! Unified parameter binding resolves sequence casting issue.The change from hardcoded parameter types to dynamic type determination using
getPDOType()correctly addresses the PostgreSQL sequence binding issue described in the PR objectives. This ensures proper type mapping for all parameter values, including the problematic empty string scenarios.
1768-1769: LGTM! Consistent with unified parameter binding approach.These changes correctly remove special-case handling for sequence parameters, allowing the unified
getPDOType()method to handle type determination dynamically. This is consistent with the overall refactoring to address PostgreSQL sequence binding issues.
# Conflicts: # src/Database/Adapter/Postgres.php
Postgres has an issue on bindValue since $sequence is an integer and we send an empty string , so he does not convert empty string to 0 .
Fix issue in Appwrite
appwrite/appwrite#10226
We should not query from the first Place when we have empty data
Summary by CodeRabbit