Backup hosts #2125
Draft
Backup hosts #2125
+994
−670
Conversation
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
📝 WalkthroughWalkthroughThe PR refactors backups from a config-driven disk model to a database-backed BackupHost with a plugin-style BackupAdapter architecture. It adds BackupAdapterSchemaInterface, a registry service, Wings and S3 schema implementations, a BackupHost model/resource/migration, updates services/controllers to delegate to adapters, and removes legacy BackupManager/S3Filesystem and related config entries. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Client
participant Initiator as InitiateBackupService
participant AdapterSvc as BackupAdapterService
participant Schema as BackupSchema<br/>(S3 or Wings)
participant DB as Database
participant External as External System<br/>(S3 or Wings)
User->>Initiator: initiate backup request
Initiator->>DB: create Backup record (backup_host_id)
Initiator->>AdapterSvc: get(backupHost.schema)
AdapterSvc-->>Initiator: BackupSchema instance
Initiator->>Schema: createBackup(backup)
Schema->>External: perform storage action (upload/daemon)
External-->>Schema: confirm
Schema-->>Initiator: completion
Initiator-->>User: backup created
sequenceDiagram
participant Client as Client/User
participant Controller as BackupController
participant AdapterSvc as BackupAdapterService
participant Schema as BackupSchema<br/>(S3 or Wings)
participant External as External System
Client->>Controller: request download link
Controller->>AdapterSvc: get(backup.backupHost.schema)
AdapterSvc-->>Controller: BackupSchema instance
Controller->>Schema: getDownloadLink(backup, user)
alt S3
Schema->>External: generate presigned URL
else Wings
Schema->>External: generate JWT and daemon URL
end
External-->>Schema: url
Schema-->>Controller: download URL
Controller-->>Client: return URL
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In `@app/Extensions/BackupAdapter/Schemas/S3BackupSchema.php`:
- Around line 106-154: In getUploadParts, accessing
$backup->backupHost->configuration['storage_class'] directly can emit warnings
when that key is missing; change the $storageClass assignment to safely read the
key (e.g. $storageClass = $backup->backupHost->configuration['storage_class'] ??
null or use array_key_exists) and keep the subsequent null check (if
(!is_null($storageClass)) { $params['StorageClass'] = $storageClass; }) so
missing config no longer triggers PHP notices.
- Around line 19-36: The S3 client is using wrong config keys and a single
cached client for all hosts; in S3BackupSchema::createClient(BackupHost
$backupHost) map the form fields to AWS SDK names (map access_key → key,
secret_key → secret, default_region → region) and preserve token if present,
build credentials via Arr::only on those mapped keys, and include region/version
in $config before constructing S3Client; change the client cache to be per-host
(keyed by $backupHost->id) instead of the single ?S3Client $client to avoid
cross-host credential leakage; also ensure any use sites like getUploadParts
read and respect storage_class from the host configuration (defaulting if
absent).
In `@app/Filament/Admin/Resources/BackupHosts/Pages/EditBackupHost.php`:
- Around line 27-31: The DeleteAction in EditBackupHost.php currently only
disables deletion when the BackupHost has associated backups; update
DeleteAction::make() so it also disables (and adjusts its label) when
BackupHost::count() === 1 to prevent deleting the last host. Specifically,
change the ->disabled(...) callback to return true if
$backupHost->backups()->count() > 0 OR BackupHost::count() === 1, and update the
->label(...) callback to return a distinct translatable message when
BackupHost::count() === 1 (e.g., a "cannot delete last host" message) while
preserving the existing backups-related message; reference DeleteAction::make(),
BackupHost::backups(), and BackupHost::count() to locate the code.
In `@app/Http/Controllers/Api/Remote/Backups/BackupRemoteUploadController.php`:
- Around line 57-63: The code in BackupRemoteUploadController currently resolves
the schema from collect($node->backupHosts)->first(), which can pick the wrong
host; instead resolve the schema from the Backup model's host property (use
$backup->host or equivalent) and validate it is present before using it. Replace
the get(collect($node->backupHosts)->first()->schema) call with fetching the
host associated with $backup (guarding against null), then call
$this->backupService->get($backupHost->schema) and keep the instanceof
S3BackupSchema check; if the backup's host is null or the schema lookup fails,
throw a BadRequestHttpException.
In `@app/Http/Controllers/Api/Remote/Backups/BackupStatusController.php`:
- Around line 72-76: The current logic in BackupStatusController resolves the S3
schema using collect($node->backupHosts)->first(), which can pick the wrong host
or be null; instead resolve the schema from the backup's own host and guard
against missing hosts: call $this->backupService->get(...) using the host
associated with the backup model (the host/backupHost property on $model) and
only invoke S3BackupSchema->completeMultipartUpload($model, $successful,
$request->input('parts')) if the resolved schema is an S3BackupSchema and the
host/schema is not null.
In `@app/Services/Backups/DeleteBackupService.php`:
- Around line 34-37: The code in DeleteBackupService uses
$backup->backupHost->schema without checking whether $backup->backupHost is
null, which can cause a fatal error; update the method in DeleteBackupService to
first verify $backup->backupHost is not null (e.g., if null throw a clear
Exception or handle gracefully) before calling
$this->backupService->get($backup->backupHost->schema), and then proceed to
retrieve $schema and the subsequent logic only when the host exists.
In `@app/Services/Backups/DownloadLinkService.php`:
- Around line 22-25: The code accesses $backup->backupHost->schema without
checking if backupHost exists; add a null check in DownloadLinkService before
calling $this->backupService->get(...) so that if $backup->backupHost is null
you throw a specific exception (create e.g., MissingBackupHostException or
BackupAdapterException) instead of a generic Exception; update the method to
validate $backup->backupHost, throw the new exception with a clear message when
missing, and ensure any callers/handlers are adjusted to catch the new exception
type.
In `@app/Services/Backups/InitiateBackupService.php`:
- Around line 113-133: The current BackupHost selection (using
BackupHost::doesntHave('nodes')->orWhereHas('nodes', fn($q) => $q->where('id',
$server->node->id))->firstOrFail()) can pick a global host even when a
node-specific host exists; change the logic to first attempt to find a host
assigned to the server's node (e.g. BackupHost::whereHas('nodes', fn($q) =>
$q->where('id', $server->node->id))->first()), and if that returns null then
fall back to a global host (e.g.
BackupHost::doesntHave('nodes')->firstOrFail()); keep the remaining flow
(loading $schema, transaction, Backup::create, and $schema->createBackup) the
same but use the explicitly chosen $backupHost variable.
In `@app/Services/Servers/TransferServerService.php`:
- Line 31: The notify() method in TransferServerService.php currently ignores
the incoming $backup_uuids by overwriting them with $backups = [], causing
transfers to omit selected backups; update notify() to map/validate the provided
$backup_uuids into the $backups payload instead of emptying it (preserve
$backup_uuids, transform to the structure expected by the daemon API), ensure
you reference the $backup_uuids parameter and $backups variable inside notify(),
and adjust the payload format to match the daemon API spec (e.g., array of
objects or keyed field) before sending the transfer notification.
In `@database/migrations/2026_01_16_081858_create_backup_hosts_table.php`:
- Line 52: The migration's config array sets the 'prefix' key using the wrong
env var AWS_BACKUPS_BUCKET (copy-paste); update the 'prefix' entry to use the
dedicated environment variable (e.g., AWS_BACKUPS_PREFIX) instead of
AWS_BACKUPS_BUCKET in the migration file (create_backup_hosts_table.php) so the
'prefix' value is read from env('AWS_BACKUPS_PREFIX', '') and not from the
bucket variable.
- Around line 72-83: In the down() method the migration drops backup_hosts
before removing the backup_host_id foreign key on the backups table, which will
violate the FK constraint; update down() to first modify the backups table (in
the Schema::table('backups', ...) block) by adding the disk column, then remove
the foreign key (use $table->dropForeign(['backup_host_id']) or the explicit
constraint name) and then dropColumn('backup_host_id'), and only after that drop
the pivot table (backup_host_node) and finally drop backup_hosts (i.e., reorder
operations so foreign key is removed before dropping the referenced table).
In `@lang/en/admin/backuphost.php`:
- Line 5: The translation string 'model_label_plural' currently reads "Database
Hosts" but should be "Backup Hosts"; update the value for the
'model_label_plural' key in this translation file to "Backup Hosts" (preserve
surrounding quotes and comma) so the plural label matches the backup host
context.
🧹 Nitpick comments (7)
app/Policies/BackupHostPolicy.php (1)
21-24: Avoid N+1 queries in node authorization checks.
canTarget()can hit the DB per node; a set-based check reduces queries for hosts with many nodes.♻️ Example set-based check
- foreach ($backupHost->nodes as $node) { - if (!$user->canTarget($node)) { - return false; - } - } + $nodeIds = $backupHost->nodes->modelKeys(); + if ($nodeIds !== [] && $user->accessibleNodes()->whereIn('id', $nodeIds)->count() !== count($nodeIds)) { + return false; + }app/Models/Node.php (1)
279-282: Use canonicalbelongsToManycasing for consistency.PHP is case‑insensitive here, but consistent casing avoids IDE/static analysis confusion.
♻️ Proposed tweak
- public function backupHosts(): BelongsToMany - { - return $this->BelongsToMany(BackupHost::class); - } + /** `@return` BelongsToMany<BackupHost, $this> */ + public function backupHosts(): BelongsToMany + { + return $this->belongsToMany(BackupHost::class); + }database/migrations/2026_01_16_081858_create_backup_hosts_table.php (2)
60-64: Avoid using Eloquent models in migrations.Using
BackupHost::create()in a migration is fragile. If the model's$fillable, validation rules, or other attributes change in the future, this migration may break when run on a fresh database. Use query builder instead.Proposed fix
- $backupHost = BackupHost::create([ - 'name' => $oldDriver === 's3' ? 'Remote' : 'Local', - 'schema' => $oldDriver, - 'configuration' => $oldConfiguration, - ]); + $backupHostId = DB::table('backup_hosts')->insertGetId([ + 'name' => $oldDriver === 's3' ? 'Remote' : 'Local', + 'schema' => $oldDriver, + 'configuration' => $oldConfiguration ? json_encode($oldConfiguration) : null, + 'created_at' => now(), + 'updated_at' => now(), + ]); - DB::table('backups')->update(['backup_host_id' => $backupHost->id]); + DB::table('backups')->update(['backup_host_id' => $backupHostId]);Also remove the
use App\Models\BackupHost;import at the top.
36-41: Consider makingbackup_host_idnullable initially during migration.Adding a non-nullable foreign key column and then immediately updating all rows works, but if the backups table is large, this could cause issues. Additionally, if the migration fails partway, you may end up with rows having a
0or invalidbackup_host_id.A safer pattern is to:
- Add the column as nullable
- Create the backup host and update existing records
- Alter the column to be non-nullable
This is a minor concern given the migration context, but worth considering for robustness.
app/Services/Backups/DeleteBackupService.php (1)
16-19: Update docblock to reflect adapter-based architecture.The comment mentions "If the backup is stored in S3" but the implementation is now adapter-agnostic. Consider updating the documentation to reflect the new architecture.
Proposed update
/** - * Deletes a backup from the system. If the backup is stored in S3 a request - * will be made to delete that backup from the disk as well. + * Deletes a backup from the system. The backup adapter handles + * removing the backup data from the configured storage backend. * * `@throws` Throwable */app/Models/BackupHost.php (1)
46-50: Align casts with CarbonImmutable docblocks.Consider casting
id,created_at, andupdated_atto match the declared types and other models’ patterns.♻️ Suggested adjustment
protected function casts(): array { return [ + 'id' => 'int', 'configuration' => 'array', + 'created_at' => 'immutable_datetime', + 'updated_at' => 'immutable_datetime', ]; }app/Extensions/BackupAdapter/Schemas/WingsBackupSchema.php (1)
57-62: Use a Form/Schema component for the “no configuration” message.
TextEntryis an Infolists component andTextEntry::make(trans(...))uses a translation string as the state path. If this schema is rendered in a form, it may not display correctly. Prefer a Forms component likePlaceholder(or a Schema-compatible view field).♻️ Suggested adjustment
-use Filament\Infolists\Components\TextEntry; +use Filament\Forms\Components\Placeholder; @@ /** `@return` Component[] */ public function getConfigurationForm(): array { return [ - TextEntry::make(trans('admin/backuphost.no_configuration')), + Placeholder::make('no_configuration') + ->content(trans('admin/backuphost.no_configuration')), ]; }
app/Http/Controllers/Api/Remote/Backups/BackupRemoteUploadController.php
Show resolved
Hide resolved
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #1919
Also allows to register custom adapters (#1600)
Largely untested!!!