Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds backup policy support across the migration system: new Resource::TYPE_BACKUP_POLICY and Transfer::GROUP_BACKUPS; a Policy resource class; Source gains batching and an abstract exportGroupBackups hook; Appwrite source implements export/report/exportGroupBackups/reportBackups/exportBackupPolicies with API calls to /backups/policies and permission/plan-specific error handling; Appwrite destination adds importBackupResource, routing, validation, and create/update logic; CSV, Firebase, JSON, NHost, MockSource, and test adapters receive exportGroupBackups stubs or implementations. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Migration/Destinations/Appwrite.php`:
- Around line 213-240: The POST check for backup policy scope (in the block
guarded by Resource::TYPE_BACKUP_POLICY where call() is used against
'/backups/policies' and the $scope variable is set) currently only treats 401s
specially; update the catch in that try/catch to also handle 400 and 404 status
codes: decode the error body from $e->getMessage(), and if the code is 400 or
404, swallow the error (treat as non-fatal so migrations continue) while
preserving other behavior for 401 (keep the existing
additional_resource_not_allowed handling and Missing scope exception); leave
other throw behavior unchanged. Ensure the logic references the same $scope
variable and call() invocations so it applies to the POST validation.
In `@src/Migration/Sources/Appwrite.php`:
- Around line 184-185: The current generic exception handler around backup
policy reporting masks permission errors; update the catch block inside the
Appwrite migration code that calls reportBackups (the code that uses
filterByIdArray() with $resourceIds) to inspect the exception type/code: if the
exception indicates an authorization/permission failure (HTTP 401 or 403 from
the Appwrite client exception), surface or rethrow it (or log it as a permission
error) so missing policies.read scope is visible; for other errors (404/501 or
other API-unavailable cases) continue to treat it as "0 policies" as before. Use
the exception's getCode()/getStatusCode() and the reportBackups/filterByIdArray
symbols to locate the handler and implement the conditional handling
accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Migration/Destinations/Appwrite.php`:
- Around line 1488-1524: The POST payload omits the resourceType even though the
import code reads and validates it; update the logic in the Appwrite destination
(the block that builds $params and the conditional that checks
$resource->getResourceId() and $collection) to add a 'resourceType' key to
$params whenever you set 'resourceId' (i.e., for supported collections
determined by match on $resource->getResourceType()), so the $this->call('POST',
'/backups/policies', ...) sends both resourceId and resourceType alongside
policyId, name, services, enabled, retention and schedule.
🧹 Nitpick comments (1)
src/Migration/Destinations/Appwrite.php (1)
1497-1517: Silently droppingresourceIdfor unsupported resource types may mask bugs.When
$resource->getResourceType()returns something other thanTYPE_DATABASEorTYPE_BUCKET, the match falls through tonulland theresourceIdis silently omitted from the API call. Consider logging a warning or throwing when an unexpected resource type has a non-nullresourceId, so future additions don't silently break.♻️ Suggested approach
$collection = match ($resource->getResourceType()) { Resource::TYPE_DATABASE => 'databases', Resource::TYPE_BUCKET => 'buckets', - default => null, // Only databases and buckets support per-resource backup policies + default => throw new Exception( + resourceName: $resource->getName(), + resourceGroup: $resource->getGroup(), + resourceId: $resource->getId(), + message: 'Unsupported resource type for backup policy: ' . $resource->getResourceType(), + ), }; - - // Only pass resourceId for supported resource types - if ($collection !== null) { - $doc = $this->database->getDocument($collection, $resource->getResourceId());
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Migration/Destinations/Appwrite.php`:
- Around line 1516-1533: The code unconditionally calls
$resource->setStatus(Resource::STATUS_SUCCESS) after the API call which
overwrites any prior warning set during the per-resource type check; change the
logic around $resource->setStatus so you only set STATUS_SUCCESS when the
current status is not Resource::STATUS_WARNING (or otherwise merge/append
messages), e.g. check $resource->getStatus() (or a status flag on $resource)
after the API call and avoid calling setStatus(Resource::STATUS_SUCCESS) if it
already equals Resource::STATUS_WARNING, preserving the original warning about
unsupported resource types.
🧹 Nitpick comments (2)
src/Migration/Sources/Appwrite.php (2)
1642-1666:$resourceIdsparameter is accepted but unused.Unlike other
report*methods (e.g.,reportAuth,reportStorage) that pass$resourceIdsintobuildQueries()for filtering,reportBackupsignores it entirely. If the/backups/policiesendpoint supports query filtering, consider applying$resourceIdsto the request; otherwise, document why filtering isn't applicable here. Static analysis also flags this (PHPMDUnusedFormalParameter).
1672-1698:$batchSizeis unused — no pagination for backup policies.Every other
export*method uses$batchSizefor cursor-based pagination. Here, all policies are fetched in a single call. If the endpoint doesn't support pagination this is fine, but the unused parameter is flagged by static analysis (PHPMDUnusedFormalParameter). Consider either adding pagination or documenting why it's unnecessary (e.g., policy count is inherently bounded).
src/Migration/Sources/Appwrite.php
Outdated
| $response = $this->call('GET', '/backups/policies', [ | ||
| 'Content-Type' => 'application/json', | ||
| ]); |
There was a problem hiding this comment.
Let's add a TODO to use the SDK once these methods are there
Resolve conflicts keeping both backups and sites support.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Migration/Sources/Appwrite.php`:
- Around line 1764-1776: In the catch block that handles exceptions from
$this->call(), avoid treating non-API errors as "0 policies": after decoding
with json_decode($e->getMessage(), true) check if $body === null (or
!is_array($body)); if it's not a parsed API response then re-throw the original
exception (or throw a new exception preserving $e as previous) so
network/timeout/DNS failures are not masked, otherwise continue the existing
code-path that reads $body['code'] and only maps 401/403 to a permission
exception and fallback to setting $report[Resource::TYPE_BACKUP_POLICY] = 0 for
Cloud-feature-not-available cases.
🧹 Nitpick comments (1)
src/Migration/Sources/Appwrite.php (1)
1783-1809:$batchSizeunused; single-call fetch has no pagination.
$batchSizeis required by the abstract method signature, so it must be accepted — but this method fetches all policies in a single API call without pagination. This is fine given that backup policies are typically few, but worth noting for future scalability.Also, the default
$policy['enabled'] ?? trueon line 1802 means a missingenabledfield results in the policy being created as enabled on the destination. If the field is ever absent, this could accidentally activate policies. Consider defaulting tofalsefor safer behavior.
src/Migration/Sources/Appwrite.php
Outdated
| } catch (\Throwable $e) { | ||
| $body = \json_decode($e->getMessage(), true); | ||
| $code = $body['code'] ?? 0; | ||
|
|
||
| // Re-throw permission errors (401/403) as they indicate configuration issues | ||
| if ($code === 401 || $code === 403) { | ||
| throw new \Exception('Missing permission to access backup policies: ' . $e->getMessage(), previous: $e); | ||
| } | ||
|
|
||
| // For other errors (404/501), treat as feature not available - skip gracefully | ||
| // Backup policies are Cloud-only, may not be available on self-hosted | ||
| $report[Resource::TYPE_BACKUP_POLICY] = 0; | ||
| } |
There was a problem hiding this comment.
Network/non-API errors silently treated as "0 policies".
If $this->call() throws a non-API exception (e.g., network timeout, DNS failure), json_decode($e->getMessage(), true) returns null, $code becomes 0, and the catch falls through to setting report[...] = 0 — silently masking a connectivity problem as "no backup policies."
Consider checking $body === null (indicating a non-JSON message) before the code-based branching, and either re-throwing or logging a warning for unparseable exceptions.
Suggested fix
} catch (\Throwable $e) {
$body = \json_decode($e->getMessage(), true);
+
+ // If exception message isn't JSON, it's likely a network/system error — don't silently swallow
+ if ($body === null) {
+ throw $e;
+ }
+
$code = $body['code'] ?? 0;🤖 Prompt for AI Agents
In `@src/Migration/Sources/Appwrite.php` around lines 1764 - 1776, In the catch
block that handles exceptions from $this->call(), avoid treating non-API errors
as "0 policies": after decoding with json_decode($e->getMessage(), true) check
if $body === null (or !is_array($body)); if it's not a parsed API response then
re-throw the original exception (or throw a new exception preserving $e as
previous) so network/timeout/DNS failures are not masked, otherwise continue the
existing code-path that reads $body['code'] and only maps 401/403 to a
permission exception and fallback to setting
$report[Resource::TYPE_BACKUP_POLICY] = 0 for Cloud-feature-not-available cases.
2 participants
Summary
Summary by CodeRabbit
New Features
Behavior
Tests