Conversation
WalkthroughThis pull request introduces backup policy support to the migration framework. Changes include defining a new BACKUP_POLICY resource type, creating a Policy class to model backup policies with properties like name, services, retention, and schedule. The base Source and Destination classes are extended with backup-related methods. Appwrite source and destination are fully implemented with backup export/import logic including API calls and error handling. Four other migration sources (CSV, Firebase, JSON, NHost) receive stub exportGroupBackups methods that throw "Not Implemented". Test adapters are updated to recognize the new resource type and support backup export workflows. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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.
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