Feat: ID based query support for services. #131
Conversation
WalkthroughThe PR adds an optional array parameter Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
![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 (5)
src/Migration/Sources/Appwrite/Reader/API.php (1)
77-92: Pagination query missing limit when using cursor.When
$lastTableis set, onlyQuery::cursorAfteris added without aQuery::limit. This relies on the API's default limit. For consistency and explicit control, consider always including the limit.🔎 Proposed fix
while (true) { /* $currentTables = $this->tables->list(...); */ $currentTables = $this->database->listCollections( $databaseId, - $lastTable - ? [Query::cursorAfter($lastTable)] - : [Query::limit($pageLimit)] + $lastTable + ? [Query::limit($pageLimit), Query::cursorAfter($lastTable)] + : [Query::limit($pageLimit)] )['collections']; /* ['tables'] */src/Migration/Sources/Appwrite/Reader/Database.php (1)
47-54: Consider extracting the query-building logic to reduce duplication.This pattern for building database queries from
$resourceIdsis duplicated inAPI.php(lines 48-55). A shared helper method or trait could reduce code duplication across Reader implementations.src/Migration/Sources/Appwrite.php (3)
230-244: Potential undefined variable if first API call returns empty results.If the first call to
$this->teams->list($params)returns an empty'teams'array (line 235), then$totalTeamsis set but$lastTeambecomesnull(line 238). The loop breaks correctly, but$teamsat line 244 uses$totalTeamswhich could be 0 while$allTeamswould be empty. This works correctly, but consider adding a guard or early return if$currentTeamsis empty on the first iteration to avoid unnecessary processing.
297-307: Edge case: Empty bucket response could cause issues.If
$currentBucketsis empty on the first iteration,$bucketsremains empty, and line 302 accesses$buckets[count($buckets) - 1]['$id']which evaluates to$buckets[-1]['$id']. In PHP, this returnsnullrather than throwing an error, but it could lead to unexpected behavior in subsequent iterations if the API doesn't handlenullcursor properly.🔎 Proposed fix
while (true) { $queries = $this->buildQueries(Resource::TYPE_BUCKET, $resourceIds, $lastBucket, $pageLimit); $currentBuckets = $this->storage->listBuckets($queries)['buckets']; + if (empty($currentBuckets)) { + break; + } + $buckets = array_merge($buckets, $currentBuckets); $lastBucket = $buckets[count($buckets) - 1]['$id'] ?? null; - if (count($currentBuckets) < $pageLimit) { + if (count($currentBuckets) < $pageLimit || $lastBucket === null) { break; } }
354-368: Same edge case consideration for function pagination.The pagination loop has the same pattern where
$currentFunctions[count($currentFunctions) - 1]['$id']could access index -1 if the array is empty. Consider adding an early break when$currentFunctionsis empty for defensive coding.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/Migration/Destinations/Appwrite.phpsrc/Migration/Destinations/CSV.phpsrc/Migration/Destinations/Local.phpsrc/Migration/Sources/Appwrite.phpsrc/Migration/Sources/Appwrite/Reader.phpsrc/Migration/Sources/Appwrite/Reader/API.phpsrc/Migration/Sources/Appwrite/Reader/Database.phpsrc/Migration/Sources/CSV.phpsrc/Migration/Sources/Firebase.phpsrc/Migration/Sources/NHost.phpsrc/Migration/Sources/Supabase.phpsrc/Migration/Target.phptests/Migration/Unit/Adapters/MockDestination.phptests/Migration/Unit/Adapters/MockSource.php
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 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.
🧬 Code graph analysis (11)
src/Migration/Destinations/Appwrite.php (5)
src/Migration/Sources/Appwrite/Reader/API.php (1)
report(28-127)src/Migration/Sources/Appwrite/Reader/Database.php (1)
report(27-118)src/Migration/Destinations/CSV.php (1)
report(68-71)src/Migration/Destinations/Local.php (1)
report(79-88)src/Migration/Target.php (1)
report(71-71)
src/Migration/Sources/NHost.php (4)
src/Migration/Sources/Appwrite.php (1)
report(165-200)src/Migration/Destinations/CSV.php (1)
report(68-71)src/Migration/Destinations/Local.php (1)
report(79-88)src/Migration/Target.php (1)
report(71-71)
src/Migration/Sources/CSV.php (4)
src/Migration/Sources/Appwrite.php (1)
report(165-200)src/Migration/Sources/Appwrite/Reader/API.php (1)
report(28-127)src/Migration/Destinations/CSV.php (1)
report(68-71)src/Migration/Target.php (1)
report(71-71)
src/Migration/Sources/Appwrite/Reader.php (2)
src/Migration/Sources/Appwrite/Reader/API.php (1)
report(28-127)src/Migration/Sources/Appwrite/Reader/Database.php (1)
report(27-118)
tests/Migration/Unit/Adapters/MockSource.php (8)
src/Migration/Sources/Appwrite.php (1)
report(165-200)src/Migration/Sources/Appwrite/Reader/API.php (1)
report(28-127)src/Migration/Destinations/CSV.php (1)
report(68-71)src/Migration/Destinations/Local.php (1)
report(79-88)src/Migration/Sources/CSV.php (1)
report(72-94)src/Migration/Sources/Firebase.php (1)
report(144-170)src/Migration/Target.php (1)
report(71-71)tests/Migration/Unit/Adapters/MockDestination.php (1)
report(88-91)
src/Migration/Sources/Firebase.php (3)
src/Migration/Destinations/CSV.php (1)
report(68-71)src/Migration/Target.php (1)
report(71-71)tests/Migration/Unit/Adapters/MockDestination.php (1)
report(88-91)
src/Migration/Sources/Appwrite/Reader/API.php (2)
src/Migration/Sources/Appwrite/Reader/Database.php (2)
report(27-118)listDatabases(120-123)src/Migration/Sources/Appwrite/Reader.php (2)
report(22-22)listDatabases(30-30)
tests/Migration/Unit/Adapters/MockDestination.php (3)
src/Migration/Destinations/CSV.php (1)
report(68-71)src/Migration/Target.php (1)
report(71-71)tests/Migration/Unit/Adapters/MockSource.php (1)
report(88-91)
src/Migration/Destinations/Local.php (8)
src/Migration/Sources/Appwrite.php (1)
report(165-200)src/Migration/Sources/Appwrite/Reader/API.php (1)
report(28-127)src/Migration/Sources/Appwrite/Reader/Database.php (1)
report(27-118)src/Migration/Sources/CSV.php (1)
report(72-94)src/Migration/Sources/Firebase.php (1)
report(144-170)src/Migration/Sources/NHost.php (1)
report(103-217)src/Migration/Sources/Supabase.php (1)
report(232-347)src/Migration/Target.php (1)
report(71-71)
src/Migration/Target.php (4)
src/Migration/Sources/Appwrite.php (1)
report(165-200)src/Migration/Sources/Appwrite/Reader/API.php (1)
report(28-127)src/Migration/Sources/Appwrite/Reader/Database.php (1)
report(27-118)src/Migration/Transfer.php (1)
Transfer(5-339)
src/Migration/Sources/Appwrite.php (5)
src/Migration/Sources/Appwrite/Reader/API.php (1)
report(28-127)src/Migration/Sources/Appwrite/Reader/Database.php (1)
report(27-118)src/Migration/Sources/Appwrite/Reader.php (1)
report(22-22)src/Migration/Target.php (2)
report(71-71)validateResourceIds(192-202)src/Migration/Resource.php (1)
Resource(5-236)
🪛 PHPMD (2.15.0)
src/Migration/Destinations/Appwrite.php
139-139: Avoid unused parameters such as '$resourceIds'. (undefined)
(UnusedFormalParameter)
src/Migration/Sources/NHost.php
103-103: Avoid unused parameters such as '$resourceIds'. (undefined)
(UnusedFormalParameter)
src/Migration/Sources/CSV.php
72-72: Avoid unused parameters such as '$resources'. (undefined)
(UnusedFormalParameter)
72-72: Avoid unused parameters such as '$resourceIds'. (undefined)
(UnusedFormalParameter)
tests/Migration/Unit/Adapters/MockSource.php
88-88: Avoid unused parameters such as '$resources'. (undefined)
(UnusedFormalParameter)
88-88: Avoid unused parameters such as '$resourceIds'. (undefined)
(UnusedFormalParameter)
src/Migration/Sources/Firebase.php
144-144: Avoid unused parameters such as '$resources'. (undefined)
(UnusedFormalParameter)
144-144: Avoid unused parameters such as '$resourceIds'. (undefined)
(UnusedFormalParameter)
src/Migration/Destinations/CSV.php
68-68: Avoid unused parameters such as '$resources'. (undefined)
(UnusedFormalParameter)
68-68: Avoid unused parameters such as '$resourceIds'. (undefined)
(UnusedFormalParameter)
src/Migration/Sources/Supabase.php
232-232: Avoid unused parameters such as '$resourceIds'. (undefined)
(UnusedFormalParameter)
tests/Migration/Unit/Adapters/MockDestination.php
88-88: Avoid unused parameters such as '$resources'. (undefined)
(UnusedFormalParameter)
88-88: Avoid unused parameters such as '$resourceIds'. (undefined)
(UnusedFormalParameter)
src/Migration/Destinations/Local.php
79-79: Avoid unused parameters such as '$resources'. (undefined)
(UnusedFormalParameter)
79-79: Avoid unused parameters such as '$resourceIds'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (16)
src/Migration/Destinations/CSV.php (1)
68-71: LGTM! Signature update for interface compliance.The addition of the optional
$resourceIdsparameter aligns with the abstract method declaration inTarget.php. Destination classes typically validate capabilities rather than implement resource filtering, so the unused parameter is expected and acceptable.src/Migration/Sources/CSV.php (1)
72-94: LGTM! Signature update for interface compliance.The addition of the optional
$resourceIdsparameter aligns with the abstract method inTarget.php. Since this CSV source operates on a single file representing one resource (format:{databaseId:tableId}), resource ID filtering doesn't apply to this implementation. The signature update ensures interface compliance.src/Migration/Destinations/Appwrite.php (1)
139-209: LGTM! Signature update for interface compliance.The addition of the optional
$resourceIdsparameter aligns with the abstract method inTarget.php. This destination'sreportmethod validates API key scopes rather than enumerating resources, so the unused parameter is expected and acceptable.tests/Migration/Unit/Adapters/MockSource.php (1)
88-91: LGTM! Mock signature updated for test compatibility.The addition of the optional
$resourceIdsparameter ensures this mock class matches the abstractTarget::reportinterface. Mock implementations intentionally don't implement full logic, so the unused parameter is expected.tests/Migration/Unit/Adapters/MockDestination.php (1)
88-91: LGTM! Mock signature updated for test compatibility.The addition of the optional
$resourceIdsparameter ensures this mock class matches the abstractTarget::reportinterface. Mock implementations intentionally don't implement full logic, so the unused parameter is expected.src/Migration/Sources/NHost.php (1)
103-103: LGTM! Signature update aligns with framework-wide API extension.The addition of the
$resourceIdsparameter is consistent with the broader PR pattern extending thereportmethod signature across all migration sources and destinations. While currently unused here, this establishes a uniform API surface and enables future resource filtering capabilities.The PHPMD warning about the unused parameter is expected and acceptable for this signature-only change.
src/Migration/Sources/Appwrite/Reader.php (1)
19-22: LGTM! Clean interface extension with proper documentation.The interface signature update correctly extends the
reportmethod to accept resource IDs for filtering. The docblock accurately documents the parameter type asarray<string, array<string>>, which aligns with implementations in API.php and Database.php that leverage this parameter for database and table filtering.src/Migration/Destinations/Local.php (1)
79-79: LGTM! Signature update maintains API consistency.The addition of the
$resourceIdsparameter aligns with the framework-wide API extension. For the Local destination, this method only validates write permissions and returns an empty report, so both parameters remain unused. This is expected behavior for destination implementations.The PHPMD warnings about unused parameters are acceptable for this signature-only change required by the abstract Target class.
src/Migration/Sources/Firebase.php (1)
144-144: LGTM! Signature update establishes API consistency.The addition of the
$resourceIdsparameter maintains consistency with the framework-wide API extension. Firebase'sreportmethod focuses on validating service account credentials and OAuth scopes rather than performing resource counting, so the parameters remain unused. This is expected behavior.The PHPMD warnings about unused parameters are acceptable for this signature-only change.
src/Migration/Sources/Supabase.php (1)
232-232: LGTM! Signature update aligns with framework API.The addition of the
$resourceIdsparameter maintains consistency with the broader framework API extension. While currently unused, this establishes a uniform interface across all migration sources and positions Supabase for future resource filtering capabilities.The PHPMD warning about the unused parameter is expected and acceptable for this signature-only change.
src/Migration/Sources/Appwrite/Reader/API.php (1)
28-61: LGTM on the resourceIds filtering implementation.The filtering logic correctly handles both array and scalar values for database IDs and properly builds the query for the API call.
src/Migration/Sources/Appwrite/Reader/Database.php (1)
27-66: Implementation looks correct and consistent with the PR's goals.The filtering logic properly handles the optional
$resourceIdsparameter and builds appropriate queries for database filtering. The count and list operations correctly use the filtered queries.src/Migration/Target.php (2)
66-71: Signature update and documentation look good.The abstract method signature is properly updated with the new optional parameter, and the docblock correctly describes the parameter type and potential exception.
189-202: Validation implementation is correct.The
validateResourceIdsmethod properly validates that only top-level/root resource types are provided in the$resourceIdsarray. The error message is descriptive and includes the list of allowed resource types.src/Migration/Sources/Appwrite.php (2)
165-179: Report method correctly validates and propagates resourceIds.The implementation properly validates resource IDs early and consistently passes them to all sub-report methods. This ensures filtering is applied uniformly across auth, databases, storage, and functions.
1603-1626: Pagination with cursor missing explicit limit.When
$cursoris provided (lines 1619-1623), onlyQuery::cursorAfteris added withoutQuery::limit. This means paginated requests rely on the API's default limit rather than the specified$limitparameter. The limit should be included alongside the cursor.🔎 Proposed fix
private function buildQueries(string $resourceType, array $resourceIds, ?string $cursor = null, int $limit = 25): array { $queries = []; if (!empty($resourceIds[$resourceType])) { $ids = is_array($resourceIds[$resourceType]) ? $resourceIds[$resourceType] : [$resourceIds[$resourceType]]; $queries[] = Query::equal('$id', $ids); } + $queries[] = Query::limit($limit); + if ($cursor) { $queries[] = Query::cursorAfter($cursor); - } else { - $queries[] = Query::limit($limit); } return $queries; }⛔ Skipped due to learnings
Learnt from: ItzNotABug Repo: utopia-php/migration PR: 89 File: src/Migration/Sources/Appwrite/Reader/API.php:64-84 Timestamp: 2025-07-19T08:29:22.290Z 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.
| while (true) { | ||
| $currentBuckets = $this->storage->listBuckets( | ||
| $lastBucket | ||
| ? [Query::cursorAfter($lastBucket)] | ||
| : [Query::limit($pageLimit)] | ||
| )['buckets']; | ||
| $queries = $this->buildQueries(Resource::TYPE_BUCKET, $resourceIds, $lastBucket, $pageLimit); | ||
| $currentBuckets = $this->storage->listBuckets($queries)['buckets']; | ||
|
|
||
| $buckets = array_merge($buckets, $currentBuckets); | ||
| $lastBucket = $buckets[count($buckets) - 1]['$id'] ?? null; |
There was a problem hiding this comment.
Why don't we update the file loop as well?
There was a problem hiding this comment.
I have a PR on Appwrite side (in review) to get the bucket size on the response model itself so we can avoid file loop later just to get the sizes. Also we only do top level queries atm, so files are nested here.
There was a problem hiding this comment.
I have a PR on Appwrite side (in review) to get the bucket size on the response model itself
Do we have that info already 🤔
There was a problem hiding this comment.
we do via stats aggregation. its just not sent back via the endpoint. the worker runs hourly so thats the only tradeoff. appwrite/appwrite#11069
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Migration/Sources/Appwrite.php (1)
1640-1660: Consider documenting the limit parameter behavior with cursors.The buildQueries method works correctly in all current usage, but the
$limitparameter is ignored when$cursoris provided, relying on the Appwrite API's default limit of 25. While this works since all cursor calls use the default limit, the method signature might mislead future callers into thinking a custom limit would be respected alongside a cursor.🔎 Options to improve clarity
Option 1: Add a docblock clarifying the behavior
/** * Build queries with optional filtering by resource IDs + * + * @param string $resourceType The type of resource to query + * @param array $resourceIds Map of resource types to IDs for filtering + * @param string|null $cursor The cursor for pagination (when provided, $limit is ignored and API default of 25 is used) + * @param int $limit The page limit (only applied when $cursor is null) + * @return array The constructed query array */ private function buildQueries(string $resourceType, array $resourceIds, ?string $cursor = null, int $limit = self::DEFAULT_PAGE_LIMIT): arrayOption 2: Add limit alongside cursor (more explicit)
private function buildQueries(string $resourceType, array $resourceIds, ?string $cursor = null, int $limit = self::DEFAULT_PAGE_LIMIT): array { $queries = []; if (!empty($resourceIds[$resourceType])) { $ids = (array) $resourceIds[$resourceType]; $queries[] = Query::equal('$id', $ids); } + $queries[] = Query::limit($limit); + if ($cursor) { $queries[] = Query::cursorAfter($cursor); - } else { - $queries[] = Query::limit($limit); } return $queries; }Based on learnings, the Appwrite API uses a default limit of 25 when not specified, so option 1 (documentation) may be sufficient if you prefer to rely on the API default with cursors.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Migration/Sources/Appwrite.phpsrc/Migration/Sources/Appwrite/Reader/API.phpsrc/Migration/Sources/Appwrite/Reader/Database.php
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 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.
📚 Learning: 2025-06-28T09:47:58.757Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 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.
Applied to files:
src/Migration/Sources/Appwrite/Reader/API.php
📚 Learning: 2025-06-28T09:47:08.333Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 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.
Applied to files:
src/Migration/Sources/Appwrite/Reader/API.php
📚 Learning: 2025-07-19T08:29:22.290Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 89
File: src/Migration/Sources/Appwrite/Reader/API.php:64-84
Timestamp: 2025-07-19T08:29:22.290Z
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.
Applied to files:
src/Migration/Sources/Appwrite.php
🧬 Code graph analysis (2)
src/Migration/Sources/Appwrite/Reader/Database.php (3)
src/Migration/Sources/Appwrite/Reader/API.php (2)
report(28-125)listDatabases(130-133)src/Migration/Sources/Appwrite/Reader.php (2)
report(22-22)listDatabases(30-30)src/Migration/Resource.php (1)
Resource(5-236)
src/Migration/Sources/Appwrite/Reader/API.php (3)
src/Migration/Sources/Appwrite/Reader/Database.php (1)
report(27-116)src/Migration/Sources/Appwrite/Reader.php (1)
report(22-22)src/Migration/Resource.php (1)
Resource(5-236)
🔇 Additional comments (6)
src/Migration/Sources/Appwrite/Reader/API.php (1)
28-60: LGTM! Database filtering correctly implemented.The resourceIds integration properly constructs database queries when specific database IDs are provided, and correctly passes them to the list method. The array cast ensures type safety, and the empty check prevents unnecessary query clauses.
src/Migration/Sources/Appwrite/Reader/Database.php (1)
27-64: LGTM! Consistent database filtering implementation.The database reader correctly implements resource ID filtering by building queries and applying them to both counting and listing operations. The implementation pattern matches the API reader for consistency across reader types.
src/Migration/Sources/Appwrite.php (4)
167-202: LGTM! Resource IDs properly propagated through reporting flow.The public report method correctly accepts resourceIds, validates them, and propagates them through all the private reporting methods. This provides a clean interface for filtered resource reporting.
210-280: LGTM! Auth resource reporting correctly integrated with filtering.The auth reporting methods properly utilize buildQueries to filter users and teams when resource IDs are provided. The implementation correctly distinguishes between count-only queries (limit 1) and full iteration (with pagination) based on whether membership data is needed.
297-362: LGTM! Storage reporting correctly implements bucket filtering.The storage reporting properly uses buildQueries for bucket filtering. File queries are intentionally handled differently since files are nested resources under buckets—filtering is achieved by iterating through the already-filtered bucket list. This design aligns with the discussion in past reviews about top-level resource filtering.
364-427: LGTM! Functions reporting correctly uses query building.The function reporting properly leverages buildQueries for both count-only operations (limit 1) and full iteration with pagination. The logic correctly short-circuits when only function counts are needed, and fetches full data when deployments or environment variables are requested.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Migration/Sources/Appwrite.php (1)
1651-1655: Consider adding limit explicitly even when cursor is present.The current conditional logic adds
Query::limit($limit)only when no cursor is provided. When a cursor exists, the limit is omitted entirely, relying on the Appwrite API's implicit default of 25 records. While this works becauseDEFAULT_PAGE_LIMITmatches the API default, it has drawbacks:
- The
$limitparameter is ignored when$cursoris provided, which is unexpected given the function signature- Relies on implicit API behavior that could change
- First page uses explicit limit, but subsequent pages don't—creating inconsistency
- If someone later passes a custom limit expecting it to work with pagination, it won't
🔎 Proposed refactor to always include explicit limit
- if ($cursor) { - $queries[] = Query::cursorAfter($cursor); - } else { - $queries[] = Query::limit($limit); - } + $queries[] = Query::limit($limit); + + if ($cursor) { + $queries[] = Query::cursorAfter($cursor); + }This makes the limit explicit for all pages and honors the
$limitparameter consistently.Based on learnings, the Appwrite API defaults to 25 when using
cursorAfter(), so the current code works—but explicit is better than implicit for maintainability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Migration/Sources/Appwrite.php
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 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.
📚 Learning: 2025-07-19T08:29:22.290Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 89
File: src/Migration/Sources/Appwrite/Reader/API.php:64-84
Timestamp: 2025-07-19T08:29:22.290Z
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.
Applied to files:
src/Migration/Sources/Appwrite.php
🔇 Additional comments (4)
src/Migration/Sources/Appwrite.php (4)
55-55: LGTM! Well-chosen constant value.The
DEFAULT_PAGE_LIMITconstant with value 25 correctly matches the Appwrite API's default page limit for listings, making the pagination logic clearer and maintainable.
167-202: LGTM! Clean implementation of resource ID filtering.The addition of
$resourceIdsparameter with early validation and consistent propagation through all reporting methods follows good practices. ThevalidateResourceIdscall at the start ensures invalid inputs are caught early.
210-278: LGTM! Consistent use of buildQueries helper.The
reportAuthmethod properly usesbuildQueriesfor both counting (withlimit: 1) and pagination (with cursor), and termination conditions correctly referenceDEFAULT_PAGE_LIMIT.
1634-1658: Well-structured helper method.The
buildQuerieshelper effectively centralizes query construction for resource ID filtering and pagination. The implementation correctly:
- Filters by resource IDs when provided
- Handles cursor-based pagination
- Uses named parameters and a sensible default limit
This consolidation eliminates code duplication and makes the pagination logic easier to maintain across multiple resource types.
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.