Optimize reports fetching #89
Conversation
WalkthroughThe changes refactor internal methods for resource counting and reporting in Appwrite migration and API reader classes. Pagination is standardized with a fixed page limit and cursor-based approach, and redundant API calls are reduced by leveraging batch responses. Resource counts are now computed more efficiently and accurately using data already present in API responses. Changes
Poem
✨ 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/Migration/Sources/Appwrite.php (1)
286-334: Fix query construction in nested pagination loopsThe bucket and file pagination loops have the same query construction issue where
limitis missing when usingcursorAfter.Apply these diffs:
For bucket pagination:
$currentBuckets = $this->storage->listBuckets( $lastBucket - ? [Query::cursorAfter($lastBucket)] - : [Query::limit($pageLimit)] + ? [Query::limit($pageLimit), Query::cursorAfter($lastBucket)] + : [Query::limit($pageLimit)] )['buckets'];For file pagination:
$files = $this->storage->listFiles( $bucket['$id'], $lastFile - ? [Query::cursorAfter($lastFile)] - : [Query::limit($pageLimit)] + ? [Query::limit($pageLimit), Query::cursorAfter($lastFile)] + : [Query::limit($pageLimit)] )['files'];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Migration/Sources/Appwrite.php(5 hunks)src/Migration/Sources/Appwrite/Reader/API.php(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ItzNotABug
PR: utopia-php/migration#80
File: src/Migration/Sources/Appwrite/Reader/API.php:8-8
Timestamp: 2025-06-28T09:47:11.436Z
Learning: In the Appwrite migration codebase, commented-out Tables service references (import statements and constructor parameters) are intentionally kept for future implementation when the Tables service becomes available in the Appwrite SDK, rather than being dead code that should be removed.
Learnt from: ItzNotABug
PR: utopia-php/migration#80
File: src/Migration/Sources/Supabase.php:300-308
Timestamp: 2025-06-28T09:47:58.757Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the user ItzNotABug prefers to keep the existing query logic unchanged even if it becomes semantically incorrect with the new naming. The focus is purely on resource type renaming, not on fixing logical issues that become apparent after the terminology change.
Learnt from: ItzNotABug
PR: utopia-php/migration#80
File: src/Migration/Sources/Appwrite.php:843-851
Timestamp: 2025-06-28T09:47:08.333Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the class constructors and method parameters use the new terminology (like "relatedTable"), but the underlying data structures and API responses still use the legacy keys (like "relatedCollection"). This is an intentional design pattern to allow gradual migration while maintaining compatibility with existing data sources.
src/Migration/Sources/Appwrite/Reader/API.php (4)
Learnt from: ItzNotABug
PR: utopia-php/migration#80
File: src/Migration/Sources/Appwrite.php:843-851
Timestamp: 2025-06-28T09:47:08.333Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the class constructors and method parameters use the new terminology (like "relatedTable"), but the underlying data structures and API responses still use the legacy keys (like "relatedCollection"). This is an intentional design pattern to allow gradual migration while maintaining compatibility with existing data sources.
Learnt from: ItzNotABug
PR: utopia-php/migration#80
File: src/Migration/Sources/Supabase.php:300-308
Timestamp: 2025-06-28T09:47:58.757Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the user ItzNotABug prefers to keep the existing query logic unchanged even if it becomes semantically incorrect with the new naming. The focus is purely on resource type renaming, not on fixing logical issues that become apparent after the terminology change.
Learnt from: ItzNotABug
PR: utopia-php/migration#80
File: src/Migration/Sources/Appwrite/Reader/API.php:8-8
Timestamp: 2025-06-28T09:47:11.436Z
Learning: In the Appwrite migration codebase, commented-out Tables service references (import statements and constructor parameters) are intentionally kept for future implementation when the Tables service becomes available in the Appwrite SDK, rather than being dead code that should be removed.
Learnt from: ItzNotABug
PR: utopia-php/migration#80
File: src/Migration/Resources/Database/Columns/Relationship.php:86-89
Timestamp: 2025-06-28T09:45:57.650Z
Learning: In the utopia-php/migration codebase Relationship column class, the `getRelatedTable()` method intentionally returns `$this->options['relatedCollection']` (not `relatedTable`) because the underlying API still uses "collection" terminology, even though the internal codebase has been refactored to use "table" terminology.
🔇 Additional comments (8)
src/Migration/Sources/Appwrite/Reader/API.php (3)
93-96: Efficient column counting implementationGood optimization! Leveraging the attributes/columns already present in the table response eliminates unnecessary API calls.
98-101: Efficient index counting implementationExcellent optimization! Using the indexes already available in the table response avoids redundant API calls.
103-113: Optimal row counting given API constraintsGood use of
Query::limit(1)to minimize data transfer when only the total count is needed. The comment appropriately acknowledges the performance impact for many tables.src/Migration/Sources/Appwrite.php (5)
202-216: Smart conditional fetching of teamsGood optimization to only fetch teams when necessary based on the requested resources. The user count fetching correctly uses
Query::limit(1).
279-284: Efficient bucket countingGood optimization using
Query::limit(1)to fetch only the total count.
337-354: Excellent optimization for function countingGreat implementation of short-circuit logic when only function count is needed, avoiding unnecessary data fetching.
388-394: Efficient environment variable countingExcellent optimization using the
varsarray already present in the function model instead of making separate API calls.
1377-1377: Good defensive programmingAdding null coalescing for
deploymentIdprevents potential errors when the field is missing.
| $tables = []; | ||
| $pageLimit = 25; | ||
| $lastTable = null; | ||
|
|
||
| while (true) { | ||
| /* $currentTables = $this->tables->list(...); */ | ||
| $currentTables = $this->database->listCollections( | ||
| $databaseId, | ||
| $lastTable | ||
| ? [Query::cursorAfter($lastTable)] | ||
| : [Query::limit($pageLimit)] | ||
| )['collections']; /* ['tables'] */ | ||
|
|
||
| $tables = array_merge($tables, $currentTables); | ||
| $lastTable = $tables[count($tables) - 1]['$id'] ?? null; | ||
|
|
||
| if (count($currentTables) < $pageLimit) { | ||
| break; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix potential array access issue and query construction
There are two issues in the pagination implementation:
- Line 78 could fail if no tables are returned in the response. You should check if the array is not empty before accessing the last element.
- The query construction is incorrect. When using
cursorAfter, you still need to include thelimitparameter.
Apply this diff to fix both issues:
- $lastTable = $tables[count($tables) - 1]['$id'] ?? null;
+ $lastTable = !empty($tables) ? $tables[count($tables) - 1]['$id'] : null; $currentTables = $this->database->listCollections(
$databaseId,
$lastTable
- ? [Query::cursorAfter($lastTable)]
- : [Query::limit($pageLimit)]
+ ? [Query::limit($pageLimit), Query::cursorAfter($lastTable)]
+ : [Query::limit($pageLimit)]
)['collections']; /* ['tables'] */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $tables = []; | |
| $pageLimit = 25; | |
| $lastTable = null; | |
| while (true) { | |
| /* $currentTables = $this->tables->list(...); */ | |
| $currentTables = $this->database->listCollections( | |
| $databaseId, | |
| $lastTable | |
| ? [Query::cursorAfter($lastTable)] | |
| : [Query::limit($pageLimit)] | |
| )['collections']; /* ['tables'] */ | |
| $tables = array_merge($tables, $currentTables); | |
| $lastTable = $tables[count($tables) - 1]['$id'] ?? null; | |
| if (count($currentTables) < $pageLimit) { | |
| break; | |
| } | |
| } | |
| $tables = []; | |
| $pageLimit = 25; | |
| $lastTable = null; | |
| while (true) { | |
| /* $currentTables = $this->tables->list(...); */ | |
| $currentTables = $this->database->listCollections( | |
| $databaseId, | |
| $lastTable | |
| ? [Query::limit($pageLimit), Query::cursorAfter($lastTable)] | |
| : [Query::limit($pageLimit)] | |
| )['collections']; /* ['tables'] */ | |
| $tables = array_merge($tables, $currentTables); | |
| - $lastTable = $tables[count($tables) - 1]['$id'] ?? null; | |
| + $lastTable = !empty($tables) ? $tables[count($tables) - 1]['$id'] : null; | |
| if (count($currentTables) < $pageLimit) { | |
| break; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/Migration/Sources/Appwrite/Reader/API.php around lines 64 to 84, fix the
pagination logic by first checking if $currentTables is not empty before
accessing the last element to avoid potential errors. Also, modify the query
construction to always include the limit parameter along with cursorAfter when
$lastTable is set, ensuring correct pagination behavior.
There was a problem hiding this comment.
its fine because the $pageLimit is 25 which is the same as the default number of records returned by the API as well. We don't need - Query::limit($pageLimit), Query::cursorAfter($lastTable).
There was a problem hiding this comment.
@ItzNotABug Thank you for the clarification about the API's default behavior with 25 records. You're absolutely right that the query construction is fine as-is since the API defaults to returning 25 records.
However, there's still a potential issue with the array access on line 78. If the first API call returns an empty $currentTables array (which could happen if there are no tables in the database), then $tables would remain empty, but we'd still try to access $tables[count($tables) - 1]['$id'] where count($tables) - 1 would be -1, leading to an undefined index error.
The fix for just the array access issue would be:
$lastTable = !empty($tables) ? $tables[count($tables) - 1]['$id'] : null;But if you're confident this scenario won't occur in practice or is handled elsewhere, feel free to keep it as-is.
✏️ Learnings added
Learnt from: ItzNotABug
PR: utopia-php/migration#89
File: src/Migration/Sources/Appwrite/Reader/API.php:64-84
Timestamp: 2025-07-19T08:29:22.280Z
Learning: In the Appwrite API, the default page limit for listing collections is 25 records, so when using cursor-based pagination with Query::cursorAfter(), there's no need to explicitly specify Query::limit(25) as the API will default to this limit.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ItzNotABug
PR: utopia-php/migration#80
File: src/Migration/Sources/Supabase.php:300-308
Timestamp: 2025-06-28T09:47:58.757Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the user ItzNotABug prefers to keep the existing query logic unchanged even if it becomes semantically incorrect with the new naming. The focus is purely on resource type renaming, not on fixing logical issues that become apparent after the terminology change.
Learnt from: ItzNotABug
PR: utopia-php/migration#80
File: src/Migration/Sources/Appwrite.php:843-851
Timestamp: 2025-06-28T09:47:08.333Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the class constructors and method parameters use the new terminology (like "relatedTable"), but the underlying data structures and API responses still use the legacy keys (like "relatedCollection"). This is an intentional design pattern to allow gradual migration while maintaining compatibility with existing data sources.
Learnt from: ItzNotABug
PR: utopia-php/migration#80
File: src/Migration/Sources/Appwrite/Reader/API.php:8-8
Timestamp: 2025-06-28T09:47:11.436Z
Learning: In the Appwrite migration codebase, commented-out Tables service references (import statements and constructor parameters) are intentionally kept for future implementation when the Tables service becomes available in the Appwrite SDK, rather than being dead code that should be removed.
There was a problem hiding this comment.
kinda makes sense.
But I wonder how this never led to any issues with similar logic where pagination is used in the codebase 🤔
Changing this here would mean, changing the check everywhere we use pagination. I'll wait for @abnegate's insight on this.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
This PR does the following in order to optimize the reports fetching from Appwrite source -
Query.limit(1)when we just need thetotal.Benchmark Datasets
Benchmark Results
Summary by CodeRabbit