Open
Conversation
Contributor
WalkthroughAdds backup policy support across the migration system. Introduces Resource::TYPE_BACKUP_POLICY and a new Backups Policy resource class modeling backup policies. Adds Transfer backup group constants and integrates backups into resource extraction and public lists. Extends Source with getBackupsBatchSize and an abstract exportGroupBackups; concrete sources (Appwrite, CSV, Firebase, JSON, NHost, MockSource) add exportGroupBackups handlers (Appwrite imports via importBackupResource which marks resources skipped). Source reporting and supported-resources lists updated to include backups. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Migration/Sources/Appwrite.php (1)
1426-1437: Consider surfacing the Cloud-only no-op when backups are requested.
exportGroupBackups()currently does a silent no-op. If backups are explicitly requested on CE, a non-fatal warning/log would improve operator feedback during migration runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Migration/Sources/Appwrite.php` around lines 1426 - 1437, exportGroupBackups currently silently no-ops; change it to emit a non-fatal warning when invoked so operators know backups are Cloud-only (reference: method exportGroupBackups). Use the class logger (e.g. $this->logger or the existing logging facility in this class) to log a clear warning indicating backups are Cloud-only and will not be exported for CE, include any relevant group identifier/parameters if available. Also augment reportBackups (reference: reportBackups and Resource::TYPE_BACKUP_POLICY) to log a brief informational message when backup policies are requested and you populate the report with zero, so the migration report plus logs both surface that backups are unsupported on CE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Migration/Destinations/Appwrite.php`:
- Around line 1490-1495: The importBackupResource(Resource $resource): Resource
method currently sets Resource::STATUS_SUCCESS without performing any import;
change it to mark the resource as skipped or warning (e.g.,
Resource::STATUS_SKIPPED or Resource::STATUS_WARNING) and attach a clear message
like "Import not implemented" (or similar), and avoid returning success until
real import logic is implemented; update the method in the Appwrite migration
class (importBackupResource) to set the non-success status and message and
optionally log the skipped action so reports/do-not-report logic won't treat it
as migrated.
In `@src/Migration/Resources/Backups/Policy.php`:
- Around line 37-48: The fromArray() factory (Policy::fromArray) currently
assumes $array['id'] exists; add explicit validation for the required id: check
isset($array['id']) and that it's the expected type/format (e.g., non-empty
string or integer) and throw a clear InvalidArgumentException (or similar) if
missing/invalid so callers get a descriptive validation error instead of a PHP
notice; then pass the validated/casted id into new self(...) as before.
---
Nitpick comments:
In `@src/Migration/Sources/Appwrite.php`:
- Around line 1426-1437: exportGroupBackups currently silently no-ops; change it
to emit a non-fatal warning when invoked so operators know backups are
Cloud-only (reference: method exportGroupBackups). Use the class logger (e.g.
$this->logger or the existing logging facility in this class) to log a clear
warning indicating backups are Cloud-only and will not be exported for CE,
include any relevant group identifier/parameters if available. Also augment
reportBackups (reference: reportBackups and Resource::TYPE_BACKUP_POLICY) to log
a brief informational message when backup policies are requested and you
populate the report with zero, so the migration report plus logs both surface
that backups are unsupported on CE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c038b0e9-982e-4e7e-a15b-4ab24f9c9d3f
📒 Files selected for processing (10)
src/Migration/Destinations/Appwrite.phpsrc/Migration/Resource.phpsrc/Migration/Resources/Backups/Policy.phpsrc/Migration/Source.phpsrc/Migration/Sources/Appwrite.phpsrc/Migration/Sources/CSV.phpsrc/Migration/Sources/Firebase.phpsrc/Migration/Sources/JSON.phpsrc/Migration/Sources/NHost.phpsrc/Migration/Transfer.php
Comment on lines
+37
to
+48
| public static function fromArray(array $array): self | ||
| { | ||
| return new self( | ||
| $array['id'], | ||
| $array['name'] ?? '', | ||
| $array['services'] ?? [], | ||
| $array['retention'] ?? 0, | ||
| $array['schedule'] ?? '', | ||
| $array['enabled'] ?? true, | ||
| $array['resourceId'] ?? '', | ||
| $array['resourceType'] ?? '', | ||
| ); |
Contributor
There was a problem hiding this comment.
Harden fromArray() for missing/invalid required id.
Line 40 assumes id exists and is valid. Malformed payloads will trigger a low-level notice instead of a clear validation error.
💡 Proposed fix
public static function fromArray(array $array): self
{
+ if (!isset($array['id']) || !\is_string($array['id']) || $array['id'] === '') {
+ throw new \InvalidArgumentException('Backup policy "id" is required.');
+ }
+
return new self(
$array['id'],
$array['name'] ?? '',
$array['services'] ?? [],
$array['retention'] ?? 0,
$array['schedule'] ?? '',
$array['enabled'] ?? true,
$array['resourceId'] ?? '',
$array['resourceType'] ?? '',
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Migration/Resources/Backups/Policy.php` around lines 37 - 48, The
fromArray() factory (Policy::fromArray) currently assumes $array['id'] exists;
add explicit validation for the required id: check isset($array['id']) and that
it's the expected type/format (e.g., non-empty string or integer) and throw a
clear InvalidArgumentException (or similar) if missing/invalid so callers get a
descriptive validation error instead of a PHP notice; then pass the
validated/casted id into new self(...) as before.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Migration/Destinations/Appwrite.php (1)
1490-1495:⚠️ Potential issue | 🟡 MinorInclude a skip reason when backup import is not implemented.
Line 1492 sets
STATUS_SKIPPEDwithout context. This makes migration reports harder to interpret when backups are intentionally no-op.Proposed fix
public function importBackupResource(Resource $resource): Resource { - $resource->setStatus(Resource::STATUS_SKIPPED); + $resource->setStatus( + Resource::STATUS_SKIPPED, + 'Backup policy import is not implemented for this destination' + ); return $resource; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Migration/Destinations/Appwrite.php` around lines 1490 - 1495, In importBackupResource (class Appwrite) you should still mark the resource skipped but also record a human-readable skip reason; after setting Resource::STATUS_SKIPPED call a method on the Resource to record the reason (e.g. $resource->setSkipReason('backup import not implemented for Appwrite')) so migration reports show context — if Resource has no setSkipReason, add the reason to resource metadata (e.g. $resource->setMeta('skip_reason', 'backup import not implemented for Appwrite')) and return the resource.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Migration/Destinations/Appwrite.php`:
- Around line 1490-1495: In importBackupResource (class Appwrite) you should
still mark the resource skipped but also record a human-readable skip reason;
after setting Resource::STATUS_SKIPPED call a method on the Resource to record
the reason (e.g. $resource->setSkipReason('backup import not implemented for
Appwrite')) so migration reports show context — if Resource has no
setSkipReason, add the reason to resource metadata (e.g.
$resource->setMeta('skip_reason', 'backup import not implemented for Appwrite'))
and return the resource.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4aa7918e-6e48-4aa6-906c-b2e688117d89
📒 Files selected for processing (2)
src/Migration/Destinations/Appwrite.phptests/Migration/Unit/Adapters/MockSource.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
backup-policyresource type andbackupsgroup to the migration frameworkPolicyresource class with properties: id, name, services, retention, schedule, enabled, resourceId, resourceTypeexportGroupBackupsabstract method toSourcewith implementations in all source classesTest plan
Summary by CodeRabbit