Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe pull request changes how type filters are constructed in two database-listing modules. In Databases/XList.php Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Greptile SummaryThis PR adds an Key changes and issues:
The fix is to merge the type-filter queries into Confidence Score: 1/5
Important Files Changed
Reviews (1): Last reviewed commit: "added a fallback isnulll" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| $databases = $dbForProject->find('databases', $this->getDatabaseTypeQueryFilters()); | ||
| $total = $includeTotal ? $dbForProject->count('databases', $queries, APP_LIMIT_COUNT) : 0; |
There was a problem hiding this comment.
find(), type filter missing from count()
The refactor introduces two separate regressions:
-
find()now only receivesgetDatabaseTypeQueryFilters()— the user-supplied$queriesarray (which carries search terms, cursor pagination, ordering, limits, etc.) is completely discarded. This means pagination, search, and ordering no longer work for the listing endpoint. -
count()still uses the original$queriesbut no longer includes the type filter. For theTablesDBsubclass this means the returned total would count all databases regardless oftype, causing the total count to be incorrect.
The type-filter query needs to be merged into $queries so that both calls receive the full combined set:
$typeFilters = $this->getDatabaseTypeQueryFilters();
$allQueries = array_merge($queries, $typeFilters);
try {
$databases = $dbForProject->find('databases', $allQueries);
$total = $includeTotal ? $dbForProject->count('databases', $allQueries, APP_LIMIT_COUNT) : 0;
}| protected function getDatabaseTypeQueryFilters(): array | ||
| { | ||
| return [DATABASE_TYPE_TABLESDB, DATABASE_TYPE_LEGACY]; | ||
| } | ||
| { | ||
| return [ | ||
| Query::or([ | ||
| Query::equal('type', [DATABASE_TYPE_TABLESDB, DATABASE_TYPE_LEGACY]), | ||
| Query::isNull('type'), | ||
| ]), | ||
| ]; | ||
| } |
There was a problem hiding this comment.
The opening brace { and method body are indented at column 0 instead of being aligned with the class body (4 spaces). This breaks the code's formatting conventions.
| protected function getDatabaseTypeQueryFilters(): array | |
| { | |
| return [DATABASE_TYPE_TABLESDB, DATABASE_TYPE_LEGACY]; | |
| } | |
| { | |
| return [ | |
| Query::or([ | |
| Query::equal('type', [DATABASE_TYPE_TABLESDB, DATABASE_TYPE_LEGACY]), | |
| Query::isNull('type'), | |
| ]), | |
| ]; | |
| } | |
| protected function getDatabaseTypeQueryFilters(): array | |
| { | |
| return [ | |
| Query::or([ | |
| Query::equal('type', [DATABASE_TYPE_TABLESDB, DATABASE_TYPE_LEGACY]), | |
| Query::isNull('type'), | |
| ]), | |
| ]; | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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)
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/XList.php (1)
24-32: Fix method indentation to comply with PSR-12.The method body is not indented within the class. The opening brace should be on the same line or next line with proper indentation, and the body should be indented consistently.
🔧 Proposed formatting fix
protected function getDatabaseTypeQueryFilters(): array -{ - return [ - Query::or([ - Query::equal('type', [DATABASE_TYPE_TABLESDB, DATABASE_TYPE_LEGACY]), - Query::isNull('type'), - ]), - ]; -} + { + return [ + Query::or([ + Query::equal('type', [DATABASE_TYPE_TABLESDB, DATABASE_TYPE_LEGACY]), + Query::isNull('type'), + ]), + ]; + }As per coding guidelines: "Follow PSR-12 coding standard".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Modules/Databases/Http/TablesDB/XList.php` around lines 24 - 32, Adjust the method indentation for getDatabaseTypeQueryFilters in class XList to comply with PSR-12: place the opening brace on the same line as the method declaration or on the next line, then indent the method body one level inside the class and align the return array elements consistently (ensure Query::or and its nested entries are indented). Update only the whitespace/formatting of getDatabaseTypeQueryFilters so the signature, brace, and return block follow PSR-12 indentation rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Appwrite/Platform/Modules/Databases/Http/Databases/XList.php`:
- Around line 100-101: The current calls to $dbForProject->find('databases',
...) and ->count('databases', ...) are using different filter sets causing user
queries (pagination, search, filters in $queries) and type filters (from
getDatabaseTypeQueryFilters()) to be applied inconsistently; fix by merging the
two filter arrays into a single filter set and use that for both operations
(e.g., combine $queries with $this->getDatabaseTypeQueryFilters() into $filters)
so that $dbForProject->find('databases', $filters) applies
cursor/pagination/search/filters and $dbForProject->count('databases', $filters,
APP_LIMIT_COUNT) applies the same type constraints for correct total counts.
---
Nitpick comments:
In `@src/Appwrite/Platform/Modules/Databases/Http/TablesDB/XList.php`:
- Around line 24-32: Adjust the method indentation for
getDatabaseTypeQueryFilters in class XList to comply with PSR-12: place the
opening brace on the same line as the method declaration or on the next line,
then indent the method body one level inside the class and align the return
array elements consistently (ensure Query::or and its nested entries are
indented). Update only the whitespace/formatting of getDatabaseTypeQueryFilters
so the signature, brace, and return block follow PSR-12 indentation rules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 72f2f747-2a8b-402f-9d81-36b9233bdd2b
📒 Files selected for processing (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/XList.phpsrc/Appwrite/Platform/Modules/Databases/Http/TablesDB/XList.php
src/Appwrite/Platform/Modules/Databases/Http/Databases/XList.php
Outdated
Show resolved
Hide resolved
🔄 PHP-Retry SummaryFlaky tests detected across commits: Commit
|
| Test | Retries | Total Time | Details |
|---|---|---|---|
LegacyConsoleClientTest::testTimeout |
1 | 120.73s | Logs |
LegacyCustomClientTest::testCreateIndexes |
1 | 242.55s | Logs |
LegacyCustomServerTest::testTimeout |
1 | 120.98s | Logs |
LegacyTransactionsConsoleClientTest::testTransactionExpiration |
1 | 240.38s | Logs |
UsageTest::testVectorsDBStats |
1 | 10.06s | Logs |
✨ Benchmark results
⚡ Benchmark Comparison
|
This comment has been minimized.
This comment has been minimized.
2 participants
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist