Conversation
WalkthroughAdds backup policy support across the migration system: introduces Resource::TYPE_BACKUP_POLICY and Transfer group GROUP_BACKUPS; adds a new Policy resource class; extends Source and Transfer to include backups grouping and batching; implements export/import and reporting for backups in Appwrite (including direct API calls and error handling); adds importBackupResource() to Appwrite destination; adds exportGroupBackups stubs to CSV, Firebase, JSON, NHost, MockSource, and updates test adapters to recognize the new resource type. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
| $this->reportBackups($resources, $report, $resourceIds); | ||
|
|
There was a problem hiding this comment.
Distinguish between API availability errors and permission errors in backup policy reporting.
The backup policy reporting uses a generic catch-all error handler (line 1665) that converts any exception into "0 policies", which masks permission errors (401/403) caused by missing policies.read scope. Since Appwrite Cloud may return different error codes than self-hosted (e.g., 404/501 for unsupported features vs. 401/403 for missing permissions), the error handling should distinguish between these cases and surface permission errors appropriately.
Note: $resourceIds is actually used via filterByIdArray() on line 1598, so no change is needed for that parameter.
Applies to: lines 1640-1669
🤖 Prompt for AI Agents
In `@src/Migration/Sources/Appwrite.php` around lines 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());
| $params = [ | ||
| 'policyId' => $resource->getId(), | ||
| 'name' => $resource->getPolicyName(), | ||
| 'services' => $resource->getServices(), | ||
| 'enabled' => $resource->getEnabled(), | ||
| 'retention' => $resource->getRetention(), | ||
| 'schedule' => $resource->getSchedule(), | ||
| ]; | ||
|
|
||
| if ($resource->getResourceId()) { | ||
| $collection = match ($resource->getResourceType()) { | ||
| Resource::TYPE_DATABASE => 'databases', | ||
| Resource::TYPE_BUCKET => 'buckets', | ||
| default => null, // Only databases and buckets support per-resource backup policies | ||
| }; | ||
|
|
||
| // Only pass resourceId for supported resource types | ||
| if ($collection !== null) { | ||
| $doc = $this->database->getDocument($collection, $resource->getResourceId()); | ||
| if ($doc->isEmpty()) { | ||
| throw new Exception( | ||
| resourceName: $resource->getName(), | ||
| resourceGroup: $resource->getGroup(), | ||
| resourceId: $resource->getId(), | ||
| message: 'Referenced ' . $resource->getResourceType() . ' "' . $resource->getResourceId() . '" not found on destination', | ||
| ); | ||
| } | ||
|
|
||
| $params['resourceId'] = $resource->getResourceId(); | ||
| } | ||
| } | ||
|
|
||
| $this->call('POST', '/backups/policies', [ | ||
| 'Content-Type' => 'application/json', | ||
| 'X-Appwrite-Project' => $this->project, | ||
| 'X-Appwrite-Key' => $this->key, | ||
| ], $params); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for /backups/policies usage to understand the API contract
rg -n "backups/policies" --type php -B 2 -A 5Repository: utopia-php/migration
Length of output: 3194
🏁 Script executed:
#!/bin/bash
# Find and examine the Policy resource class
fd "Policy.php" -type fRepository: utopia-php/migration
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Look for exportBackupResource method to understand reverse operation
rg -n "exportBackupResource|TYPE_BACKUP_POLICY" --type php -A 5 -B 2Repository: utopia-php/migration
Length of output: 8359
🏁 Script executed:
#!/bin/bash
# Find the Policy resource class file and examine its properties
find . -name "Policy.php" -path "*/Backups/*" | head -5Repository: utopia-php/migration
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Look at the exportBackupPolicies method in full
rg -n "exportBackupPolicies" -A 30 src/Migration/Sources/Appwrite.phpRepository: utopia-php/migration
Length of output: 2239
🏁 Script executed:
#!/bin/bash
# Check the Policy class properties and methods
rg -n "class Policy|public function get|private \$" src/Migration/Resources/Backups/Policy.phpRepository: utopia-php/migration
Length of output: 457
🏁 Script executed:
#!/bin/bash
# Examine the Policy class constructor and all properties
cat -n src/Migration/Resources/Backups/Policy.phpRepository: utopia-php/migration
Length of output: 3521
🏁 Script executed:
#!/bin/bash
# Check the complete importBackupResource method to see the full context
rg -n "importBackupResource" -A 50 src/Migration/Destinations/Appwrite.php | head -80Repository: utopia-php/migration
Length of output: 3726
🏁 Script executed:
#!/bin/bash
# Check if there are any tests or examples showing the expected API contract
rg -n "resourceType" src/Migration/Destinations/Appwrite.php -B 2 -A 2Repository: utopia-php/migration
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Look for any comments or documentation about the POST /backups/policies endpoint
rg -n "policies" src/Migration/Destinations/Appwrite.php -B 3 -A 3 | grep -A 5 -B 5 "POST"Repository: utopia-php/migration
Length of output: 974
🏁 Script executed:
#!/bin/bash
# Verify the complete POST call in importBackupResource to see full context
sed -n '1483,1530p' src/Migration/Destinations/Appwrite.phpRepository: utopia-php/migration
Length of output: 2276
resourceType parameter is missing from the POST /backups/policies request.
The Policy resource class stores and provides getResourceType(), and the export method reads it from the source API (line 1685 in Sources/Appwrite.php). However, the import method reads this value for validation (line 1498) but never includes it in the $params sent to the destination API. Only resourceId is conditionally added to params (line 1517), creating an asymmetry in the bidirectional data mapping.
Fix: Include resourceType in POST params when resourceId is present
$params['resourceId'] = $resource->getResourceId();
+ $params['resourceType'] = $resource->getResourceType();🤖 Prompt for AI Agents
In `@src/Migration/Destinations/Appwrite.php` around lines 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.
1 participant
Summary
backup-policyas a new resource type withPolicyresource modelpolicies.readandpolicies.writeinreport()exportGroupBackupsabstract inSourcebase class, add stubs in CSV, Firebase, JSON, NHost sourcesTest plan
policies.read/policies.writeadditional_resource_not_allowedon Starter planSummary by CodeRabbit
New Features
Behavior
Tests