Allow Exporting and Importing of servers #2144
Open
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
📝 WalkthroughWalkthroughIntroduces server configuration import/export functionality via YAML files. Adds two Filament actions for the admin UI, two services for business logic (export and import), an API controller with corresponding endpoints, and translation keys. Enables admins and API users to export server settings as YAML and create new servers from YAML configurations. Changes
Sequence Diagram(s)sequenceDiagram
actor Admin
participant ExportAction as ExportServerConfigAction
participant Service as ServerConfigExporterService
participant Storage as Storage/Disk
Admin->>ExportAction: Click export, select options
ExportAction->>Service: handle(server, options)
Service->>Storage: Read server icon if exists
Service->>Service: Build YAML structure with server config
Service-->>ExportAction: YAML string
ExportAction->>Admin: Stream YAML download (server-name.yaml)
sequenceDiagram
actor Admin
participant ImportAction as ImportServerConfigAction
participant CreatorService as ServerConfigCreatorService
participant Database as Database
participant Storage as Storage/Disk
Admin->>ImportAction: Upload YAML file, select node
ImportAction->>CreatorService: fromFile(uploadedFile, nodeId)
CreatorService->>CreatorService: Validate and parse YAML
CreatorService->>Database: Fetch Egg by UUID
CreatorService->>Database: Resolve target Node with access checks
CreatorService->>Database: Find/create Allocations (IP/Port discovery)
CreatorService->>Database: Create Server with defaults
CreatorService->>Database: Associate Allocations to Server
CreatorService->>Storage: Decode and store server icon
CreatorService->>Database: Import Variables to ServerVariables
CreatorService-->>ImportAction: Server created
ImportAction->>Admin: Success notification + redirect to edit page
Possibly related PRs
🚥 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. 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: 9
🤖 Fix all issues with AI agents
In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php`:
- Around line 1014-1016: Replace the hardcoded hint string on
ToggleButtons::make('export_help') with a trans() call (e.g.,
trans('admin/server.import_export.export_help')) and ensure the corresponding
English translation key import_export.export_help is added to the admin server
translations with the text "Export server configuration to a YAML file that can
be imported on another panel".
In `@app/Filament/Components/Actions/ImportServerConfigAction.php`:
- Around line 51-58: The Select field may be hidden so its default can be
missing; in ImportServerConfigAction (the action handler method that processes
$data in this class) ensure you fall back to the user's first accessible node
when node_id is null: if $data['node_id'] is empty, set it to
user()?->accessibleNodes()->first()?->id (and validate existence and throw a
clear error if no accessible node exists) before proceeding with the import
logic.
In `@app/Http/Controllers/Api/Application/Servers/ServerConfigController.php`:
- Around line 60-65: The create method in ServerConfigController lacks
authorization and relies on Request and user()?->accessibleNodes(), which can
fail for API token auth; replace the generic Request with a new FormRequest
(e.g., CreateServerFromConfigRequest extending ApplicationApiRequest) that sets
protected ?string $resource = Server::RESOURCE_NAME and protected int
$permission = AdminAcl::WRITE and implements the same rules for 'file' and
'node_id'; update the controller create(Request $request) signature to
create(CreateServerFromConfigRequest $request) and ensure
ServerConfigCreatorService is fed the authorized user or their accessible node
IDs from the validated request (not via user() helper) so node access is
enforced for API token requests.
In `@app/Services/Servers/Sharing/ServerConfigCreatorService.php`:
- Around line 225-239: The importVariables method can dereference a null
$eggVariable when calling ServerVariable::create; add a null-check after
retrieving $eggVariable in importVariables
(ServerConfigCreatorService::importVariables) and skip (continue) creating the
ServerVariable if $eggVariable is null, optionally logging a warning or debug
message with the env variable name; mirror the null-handling approach used in
ServerConfigImporterService to avoid the null pointer dereference when an egg
variable is missing.
- Around line 45-186: Wrap the entire createServer workflow in a single DB
transaction to ensure atomicity: move the allocation creation logic
(Allocation::create and any findNextAvailablePort calls), the Server::create
call, the allocation->update calls that set server_id, and the subsequent
importVariables($server, ...) and importServerIcon($server, ...) into a
DB::transaction closure (or use DB::beginTransaction/commit/rollback) so any
exception will rollback all changes; ensure you reference the existing
createServer method, Allocation::create, Server::create,
$primaryAllocation->update, and the importVariables/importServerIcon calls and
let exceptions bubble up to trigger the rollback.
- Around line 144-146: The uuid_short is being set to a full UUID (36 chars) in
Server::create, which will violate the varchar(8) UNIQUE constraint; update
ServerConfigCreatorService so uuid_short uses the first 8 characters of the
generated UUID (consistent with ServerCreationService) instead of the full
string when creating the server record (i.e., generate the UUID once and set
'uuid_short' to its first 8 chars prior to calling Server::create).
In `@app/Services/Servers/Sharing/ServerConfigImporterService.php`:
- Around line 190-193: Replace the hardcoded error string thrown in
ServerConfigImporterService (the throw new InvalidFileUploadException("Could not
find an available port for IP {$ip} starting from port {$startPort}")) with a
translation lookup using the appropriate translation key and passing the ip and
startPort as parameters (e.g. use trans()/__() with placeholders for ip and
startPort) so the exception message is localized and consistent with other
messages.
- Around line 22-29: Replace the hardcoded exception messages in
ServerConfigImporterService (the upload error check that throws
InvalidFileUploadException and the YAML parse catch that throws
InvalidFileUploadException) with trans() calls using translation keys consistent
with ServerConfigCreatorService; e.g. throw new
InvalidFileUploadException(trans('servers.import.upload_failed')) for the
$file->getError() case and throw new
InvalidFileUploadException(trans('servers.import.parse_failed', ['message' =>
$exception->getMessage()])) in the catch block, and add those keys to the locale
files so localization works the same way as ServerConfigCreatorService.
- Around line 53-65: Replace the hard-coded exception messages in
ServerConfigImporterService (the checks around $eggUuid and the $egg lookup)
with trans() calls using the existing admin/server.import_errors keys; throw
InvalidFileUploadException with
trans('admin/server.import_errors.egg_uuid_required') when $eggUuid is missing,
and when $egg is not found throw InvalidFileUploadException with
trans('admin/server.import_errors.egg_not_found', ['uuid' => $eggUuid, 'name' =>
$eggName ?? '']) (or omit name if null) so the UUID and optional name are passed
as placeholders to the translation.
🧹 Nitpick comments (5)
app/Services/Servers/Sharing/ServerConfigExporterService.php (1)
14-18: Potential N+1 query issue when Server instance is passed directly.When a
Serverinstance is passed (rather than an ID), the relationships (egg,allocations,serverVariables.variable) may not be loaded, causing lazy loading and potential N+1 queries. Consider ensuring relationships are loaded regardless of input type.♻️ Suggested fix
public function handle(Server|int $server, array $options = []): string { if (!$server instanceof Server) { $server = Server::with(['egg', 'allocations', 'serverVariables.variable'])->findOrFail($server); + } else { + $server->loadMissing(['egg', 'allocations', 'serverVariables.variable']); }app/Services/Servers/Sharing/ServerConfigCreatorService.php (1)
245-264: Consider optimizing port search with a single query.The current implementation makes one database query per port, which could be slow if many ports are allocated. A single query to find the first available port would be more efficient.
♻️ Optimized approach
protected function findNextAvailablePort(int $nodeId, string $ip, int $startPort): int { $maxPort = 65535; $usedPorts = Allocation::where('node_id', $nodeId) ->where('ip', $ip) ->whereBetween('port', [$startPort + 1, $maxPort]) ->pluck('port') ->toArray(); for ($port = $startPort + 1; $port <= $maxPort; $port++) { if (!in_array($port, $usedPorts, true)) { return $port; } } throw new InvalidFileUploadException(trans('admin/server.import_errors.port_exhausted_desc', ['ip' => $ip, 'port' => $startPort])); }app/Services/Servers/Sharing/ServerConfigImporterService.php (1)
67-86: Consolidate description into the main update call.The description is updated in a separate call (lines 84-86), which could be merged into the main update to reduce database operations.
♻️ Proposed fix
$server->update([ 'egg_id' => $egg->id, 'startup' => Arr::get($config, 'settings.startup', $server->startup), 'image' => Arr::get($config, 'settings.image', $server->image), 'skip_scripts' => Arr::get($config, 'settings.skip_scripts', $server->skip_scripts), 'memory' => Arr::get($config, 'limits.memory', $server->memory), 'swap' => Arr::get($config, 'limits.swap', $server->swap), 'disk' => Arr::get($config, 'limits.disk', $server->disk), 'io' => Arr::get($config, 'limits.io', $server->io), 'cpu' => Arr::get($config, 'limits.cpu', $server->cpu), 'threads' => Arr::get($config, 'limits.threads', $server->threads), 'oom_killer' => Arr::get($config, 'limits.oom_killer', $server->oom_killer), 'database_limit' => Arr::get($config, 'feature_limits.databases', $server->database_limit), 'allocation_limit' => Arr::get($config, 'feature_limits.allocations', $server->allocation_limit), 'backup_limit' => Arr::get($config, 'feature_limits.backups', $server->backup_limit), + 'description' => Arr::get($config, 'description', $server->description), ]); - - if (isset($config['description'])) { - $server->update(['description' => $config['description']]); - }app/Http/Controllers/Api/Application/Servers/ServerConfigController.php (1)
62-63: YAML MIME type validation may reject valid files.The
mimes:yaml,ymlvalidation relies on file extension mapping and may not work reliably. YAML files can be reported with various MIME types (text/plain,text/yaml,application/x-yaml,text/x-yaml).Compare with
ImportServerConfigActionwhich uses explicit MIME types:->acceptedFileTypes(['application/x-yaml', 'text/yaml', 'text/x-yaml', '.yaml', '.yml'])Suggested fix
$request->validate([ - 'file' => 'required|file|mimes:yaml,yml|max:1024', + 'file' => 'required|file|max:1024', 'node_id' => 'sometimes|integer|exists:nodes,id', ]); + + $file = $request->file('file'); + $extension = strtolower($file->getClientOriginalExtension()); + if (!in_array($extension, ['yaml', 'yml'])) { + throw new InvalidFileUploadException('File must be a YAML file (.yaml or .yml)'); + }app/Filament/Components/Actions/ExportServerConfigAction.php (1)
52-60: Filename prefix inconsistent with API controller.The Filament action generates filenames with
'server-'prefix (Line 56), whileServerConfigController::export()uses'server-config-'prefix. This inconsistency may confuse users who use both the UI and API.Suggested fix for consistency
$this->action(fn (ServerConfigExporterService $service, Server $server, array $data) => response()->streamDownload( function () use ($service, $server, $data) { echo $service->handle($server, $data); }, - 'server-' . str($server->name)->kebab()->lower()->trim() . '.yaml', + 'server-config-' . str($server->name)->kebab()->lower()->trim() . '.yaml', [ 'Content-Type' => 'application/x-yaml', ] ));
Comment on lines
+1014
to
+1016
| ToggleButtons::make('export_help') | ||
| ->hiddenLabel() | ||
| ->hint('Export server configuration to a YAML file that can be imported on another panel'), |
There was a problem hiding this comment.
Hardcoded string should use translation.
The hint text on Line 1016 is hardcoded in English, while all other similar hints in this file use trans() for localization (e.g., trans('admin/server.transfer_help') on Line 1006, trans('admin/server.reinstall_help') on Line 1048).
Suggested fix
ToggleButtons::make('export_help')
->hiddenLabel()
- ->hint('Export server configuration to a YAML file that can be imported on another panel'),
+ ->hint(trans('admin/server.import_export.export_help')),Ensure the translation key is added to lang/en/admin/server.php:
'import_export' => [
// ... existing keys ...
'export_help' => 'Export server configuration to a YAML file that can be imported on another panel',
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ToggleButtons::make('export_help') | |
| ->hiddenLabel() | |
| ->hint('Export server configuration to a YAML file that can be imported on another panel'), | |
| ToggleButtons::make('export_help') | |
| ->hiddenLabel() | |
| ->hint(trans('admin/server.import_export.export_help')), |
🤖 Prompt for AI Agents
In `@app/Filament/Admin/Resources/Servers/Pages/EditServer.php` around lines 1014
- 1016, Replace the hardcoded hint string on ToggleButtons::make('export_help')
with a trans() call (e.g., trans('admin/server.import_export.export_help')) and
ensure the corresponding English translation key import_export.export_help is
added to the admin server translations with the text "Export server
configuration to a YAML file that can be imported on another panel".
Comment on lines
+51
to
+58
| Select::make('node_id') | ||
| ->label(trans('admin/server.import_export.node_select')) | ||
| ->hint(trans('admin/server.import_export.node_select_hint')) | ||
| ->options(fn () => user()?->accessibleNodes()->pluck('name', 'id') ?? []) | ||
| ->searchable() | ||
| ->required() | ||
| ->visible(fn () => (user()?->accessibleNodes()->count() ?? 0) > 1) | ||
| ->default(fn () => user()?->accessibleNodes()->first()?->id), |
There was a problem hiding this comment.
Node ID may be null when only one node exists.
When the user has exactly one accessible node, the Select field is hidden (visible returns false on line 57), but the default value may not be applied for hidden fields. This could result in $data['node_id'] being null even though a node exists.
🐛 Proposed fix - ensure node_id fallback in action handler
$this->action(function (ServerConfigCreatorService $createService, array $data): void {
/** `@var` UploadedFile $file */
$file = $data['file'];
- $nodeId = $data['node_id'] ?? null;
+ $nodeId = $data['node_id'] ?? user()?->accessibleNodes()->first()?->id;🤖 Prompt for AI Agents
In `@app/Filament/Components/Actions/ImportServerConfigAction.php` around lines 51
- 58, The Select field may be hidden so its default can be missing; in
ImportServerConfigAction (the action handler method that processes $data in this
class) ensure you fall back to the user's first accessible node when node_id is
null: if $data['node_id'] is empty, set it to
user()?->accessibleNodes()->first()?->id (and validate existence and throw a
clear error if no accessible node exists) before proceeding with the import
logic.
Comment on lines
+60
to
+65
| public function create(Request $request): JsonResponse | ||
| { | ||
| $request->validate([ | ||
| 'file' => 'required|file|mimes:yaml,yml|max:1024', | ||
| 'node_id' => 'sometimes|integer|exists:nodes,id', | ||
| ]); |
There was a problem hiding this comment.
Missing authorization check for server creation.
The create method uses a generic Request instead of a custom FormRequest with proper authorization. Unlike the export method which uses GetServerRequest (with AdminAcl::READ), the create endpoint lacks permission verification.
Additionally, the node_id validation only checks existence but doesn't verify the API user has access to that node. While ServerConfigCreatorService uses user()?->accessibleNodes(), this relies on the web session's user() helper which may not work correctly for API token-based requests.
Suggested fix
Create a dedicated request class with proper authorization:
// app/Http/Requests/Api/Application/Servers/CreateServerFromConfigRequest.php
class CreateServerFromConfigRequest extends ApplicationApiRequest
{
protected ?string $resource = Server::RESOURCE_NAME;
protected int $permission = AdminAcl::WRITE;
public function rules(): array
{
return [
'file' => 'required|file|max:1024',
'node_id' => 'sometimes|integer|exists:nodes,id',
];
}
}Then use it in the controller:
- public function create(Request $request): JsonResponse
+ public function create(CreateServerFromConfigRequest $request): JsonResponse
{
- $request->validate([
- 'file' => 'required|file|mimes:yaml,yml|max:1024',
- 'node_id' => 'sometimes|integer|exists:nodes,id',
- ]);🤖 Prompt for AI Agents
In `@app/Http/Controllers/Api/Application/Servers/ServerConfigController.php`
around lines 60 - 65, The create method in ServerConfigController lacks
authorization and relies on Request and user()?->accessibleNodes(), which can
fail for API token auth; replace the generic Request with a new FormRequest
(e.g., CreateServerFromConfigRequest extending ApplicationApiRequest) that sets
protected ?string $resource = Server::RESOURCE_NAME and protected int
$permission = AdminAcl::WRITE and implements the same rules for 'file' and
'node_id'; update the controller create(Request $request) signature to
create(CreateServerFromConfigRequest $request) and ensure
ServerConfigCreatorService is fed the authorized user or their accessible node
IDs from the validated request (not via user() helper) so node access is
enforced for API token requests.
Comment on lines
+45
to
+186
| protected function createServer(array $config, ?int $nodeId = null): Server | ||
| { | ||
| $eggUuid = Arr::get($config, 'egg.uuid'); | ||
| $eggName = Arr::get($config, 'egg.name'); | ||
|
|
||
| if (!$eggUuid) { | ||
| throw new InvalidFileUploadException(trans('admin/server.import_errors.egg_uuid_required')); | ||
| } | ||
|
|
||
| $egg = Egg::where('uuid', $eggUuid)->first(); | ||
|
|
||
| if (!$egg) { | ||
| throw new InvalidFileUploadException( | ||
| trans('admin/server.import_errors.egg_not_found_desc', [ | ||
| 'uuid' => $eggUuid, | ||
| 'name' => $eggName ?: trans('admin/server.none'), | ||
| ]) | ||
| ); | ||
| } | ||
|
|
||
| if ($nodeId) { | ||
| $node = Node::whereIn('id', user()?->accessibleNodes()->pluck('id')) | ||
| ->where('id', $nodeId) | ||
| ->first(); | ||
|
|
||
| if (!$node) { | ||
| throw new InvalidFileUploadException(trans('admin/server.import_errors.node_not_accessible')); | ||
| } | ||
| } else { | ||
| $node = Node::whereIn('id', user()?->accessibleNodes()->pluck('id'))->first(); | ||
|
|
||
| if (!$node) { | ||
| throw new InvalidFileUploadException(trans('admin/server.import_errors.no_nodes')); | ||
| } | ||
| } | ||
|
|
||
| $allocations = Arr::get($config, 'allocations', []); | ||
| $primaryAllocation = null; | ||
| $createdAllocations = []; | ||
|
|
||
| if (!empty($allocations)) { | ||
| foreach ($allocations as $allocationData) { | ||
| $ip = Arr::get($allocationData, 'ip'); | ||
| $port = Arr::get($allocationData, 'port'); | ||
| $isPrimary = Arr::get($allocationData, 'is_primary', false); | ||
|
|
||
| $allocation = Allocation::where('node_id', $node->id) | ||
| ->where('ip', $ip) | ||
| ->where('port', $port) | ||
| ->whereNull('server_id') | ||
| ->first(); | ||
|
|
||
| if (!$allocation) { | ||
| $existingAllocation = Allocation::where('node_id', $node->id) | ||
| ->where('ip', $ip) | ||
| ->where('port', $port) | ||
| ->first(); | ||
|
|
||
| if ($existingAllocation) { | ||
| $port = $this->findNextAvailablePort($node->id, $ip, $port); | ||
| } | ||
|
|
||
| $allocation = Allocation::create([ | ||
| 'node_id' => $node->id, | ||
| 'ip' => $ip, | ||
| 'port' => $port, | ||
| ]); | ||
| } | ||
|
|
||
| $createdAllocations[] = $allocation; | ||
|
|
||
| if ($isPrimary && !$primaryAllocation) { | ||
| $primaryAllocation = $allocation; | ||
| } | ||
| } | ||
|
|
||
| if (!$primaryAllocation && !empty($createdAllocations)) { | ||
| $primaryAllocation = $createdAllocations[0]; | ||
| } | ||
| } | ||
|
|
||
| $owner = user(); | ||
|
|
||
| if (!$owner) { | ||
| throw new InvalidFileUploadException(trans('admin/server.import_errors.no_user')); | ||
| } | ||
|
|
||
| $serverName = Arr::get($config, 'name', 'Imported Server'); | ||
|
|
||
| $startupCommand = Arr::get($config, 'settings.startup'); | ||
| if ($startupCommand === null) { | ||
| $startupCommand = array_values($egg->startup_commands)[0]; | ||
| } | ||
|
|
||
| $dockerImage = Arr::get($config, 'settings.image'); | ||
| if ($dockerImage === null) { | ||
| $dockerImage = array_values($egg->docker_images)[0]; | ||
| } | ||
|
|
||
| $server = Server::create([ | ||
| 'uuid' => Str::uuid()->toString(), | ||
| 'uuid_short' => Str::uuid()->toString(), | ||
| 'name' => $serverName, | ||
| 'description' => Arr::get($config, 'description', ''), | ||
| 'owner_id' => $owner->id, | ||
| 'node_id' => $node->id, | ||
| 'allocation_id' => $primaryAllocation?->id, | ||
| 'egg_id' => $egg->id, | ||
| 'startup' => $startupCommand, | ||
| 'image' => $dockerImage, | ||
| 'skip_scripts' => Arr::get($config, 'settings.skip_scripts', false), | ||
| 'memory' => Arr::get($config, 'limits.memory', 512), | ||
| 'swap' => Arr::get($config, 'limits.swap', 0), | ||
| 'disk' => Arr::get($config, 'limits.disk', 1024), | ||
| 'io' => Arr::get($config, 'limits.io', 500), | ||
| 'cpu' => Arr::get($config, 'limits.cpu', 0), | ||
| 'threads' => Arr::get($config, 'limits.threads'), | ||
| 'oom_killer' => Arr::get($config, 'limits.oom_killer', false), | ||
| 'database_limit' => Arr::get($config, 'feature_limits.databases', 0), | ||
| 'allocation_limit' => Arr::get($config, 'feature_limits.allocations', 0), | ||
| 'backup_limit' => Arr::get($config, 'feature_limits.backups', 0), | ||
| ]); | ||
|
|
||
| if ($primaryAllocation) { | ||
| $primaryAllocation->update(['server_id' => $server->id]); | ||
| } | ||
|
|
||
| foreach ($createdAllocations as $allocation) { | ||
| if ($allocation->id !== $primaryAllocation?->id) { | ||
| $allocation->update(['server_id' => $server->id]); | ||
| } | ||
| } | ||
|
|
||
| if (isset($config['variables'])) { | ||
| $this->importVariables($server, $config['variables']); | ||
| } | ||
|
|
||
| if (isset($config['icon'])) { | ||
| $this->importServerIcon($server, $config['icon']); | ||
| } | ||
|
|
||
| return $server; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider wrapping server creation in a database transaction.
The createServer method creates allocations, a server, updates allocations, and imports variables/icons across multiple database operations. If any step fails mid-way, orphan allocations or inconsistent state could result. Wrapping in DB::transaction() would ensure atomicity.
♻️ Suggested approach
+use Illuminate\Support\Facades\DB;
+
protected function createServer(array $config, ?int $nodeId = null): Server
{
+ return DB::transaction(function () use ($config, $nodeId) {
// ... existing implementation ...
+ });
}🤖 Prompt for AI Agents
In `@app/Services/Servers/Sharing/ServerConfigCreatorService.php` around lines 45
- 186, Wrap the entire createServer workflow in a single DB transaction to
ensure atomicity: move the allocation creation logic (Allocation::create and any
findNextAvailablePort calls), the Server::create call, the allocation->update
calls that set server_id, and the subsequent importVariables($server, ...) and
importServerIcon($server, ...) into a DB::transaction closure (or use
DB::beginTransaction/commit/rollback) so any exception will rollback all
changes; ensure you reference the existing createServer method,
Allocation::create, Server::create, $primaryAllocation->update, and the
importVariables/importServerIcon calls and let exceptions bubble up to trigger
the rollback.
Comment on lines
+144
to
+146
| $server = Server::create([ | ||
| 'uuid' => Str::uuid()->toString(), | ||
| 'uuid_short' => Str::uuid()->toString(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find schema/migration files defining uuid_short column
find . -type f \( -name "*.php" -path "*/migrations/*" -o -name "*.php" -path "*/database/*" \) | head -20Repository: pelican-dev/panel
Length of output: 915
🏁 Script executed:
# Search for uuid_short references to understand its intended format
rg "uuid_short" --type=php -B2 -A2Repository: pelican-dev/panel
Length of output: 28244
🏁 Script executed:
# Check the Server model for any casting or validation rules
rg -A 50 "class Server" --type=php | grep -A 50 "extends"Repository: pelican-dev/panel
Length of output: 50374
🏁 Script executed:
# Find the migration that creates/defines the servers table
find . -path "*/database/migrations/*" -name "*.php" -type f | xargs grep -l "servers" | head -5Repository: pelican-dev/panel
Length of output: 446
🏁 Script executed:
# Search for the servers table creation in migrations
rg "Schema::create\('servers'|Schema::table\('servers'" -A 30 --type=phpRepository: pelican-dev/panel
Length of output: 50373
Incorrect uuid_short generation - must be 8 characters, not a full UUID.
The uuid_short column is defined as varchar(8) with a UNIQUE constraint. The current code assigns a full UUID string (36 characters), which will violate the database constraint and cause server creation to fail. Use the first 8 characters of the UUID, matching the implementation in ServerCreationService.
🐛 Proposed fix
$server = Server::create([
'uuid' => Str::uuid()->toString(),
- 'uuid_short' => Str::uuid()->toString(),
+ 'uuid_short' => substr($uuid, 0, 8),🤖 Prompt for AI Agents
In `@app/Services/Servers/Sharing/ServerConfigCreatorService.php` around lines 144
- 146, The uuid_short is being set to a full UUID (36 chars) in Server::create,
which will violate the varchar(8) UNIQUE constraint; update
ServerConfigCreatorService so uuid_short uses the first 8 characters of the
generated UUID (consistent with ServerCreationService) instead of the full
string when creating the server record (i.e., generate the UUID once and set
'uuid_short' to its first 8 chars prior to calling Server::create).
Comment on lines
+225
to
+239
| protected function importVariables(Server $server, array $variables): void | ||
| { | ||
| foreach ($variables as $variable) { | ||
| $envVariable = Arr::get($variable, 'env_variable'); | ||
| $value = Arr::get($variable, 'value'); | ||
|
|
||
| /** @var EggVariable $eggVariable */ | ||
| $eggVariable = $server->egg->variables()->where('env_variable', $envVariable)->first(); | ||
|
|
||
| ServerVariable::create([ | ||
| 'server_id' => $server->id, | ||
| 'variable_id' => $eggVariable->id, | ||
| 'variable_value' => $value, | ||
| ]); | ||
| } |
There was a problem hiding this comment.
Null pointer dereference when egg variable doesn't exist.
If $eggVariable is not found (returns null), accessing $eggVariable->id on line 236 will throw an error. The ServerConfigImporterService correctly handles this with a null check.
🐛 Proposed fix
protected function importVariables(Server $server, array $variables): void
{
foreach ($variables as $variable) {
$envVariable = Arr::get($variable, 'env_variable');
$value = Arr::get($variable, 'value');
/** `@var` EggVariable $eggVariable */
$eggVariable = $server->egg->variables()->where('env_variable', $envVariable)->first();
+ if (!$eggVariable) {
+ continue;
+ }
+
ServerVariable::create([
'server_id' => $server->id,
'variable_id' => $eggVariable->id,
'variable_value' => $value,
]);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| protected function importVariables(Server $server, array $variables): void | |
| { | |
| foreach ($variables as $variable) { | |
| $envVariable = Arr::get($variable, 'env_variable'); | |
| $value = Arr::get($variable, 'value'); | |
| /** @var EggVariable $eggVariable */ | |
| $eggVariable = $server->egg->variables()->where('env_variable', $envVariable)->first(); | |
| ServerVariable::create([ | |
| 'server_id' => $server->id, | |
| 'variable_id' => $eggVariable->id, | |
| 'variable_value' => $value, | |
| ]); | |
| } | |
| protected function importVariables(Server $server, array $variables): void | |
| { | |
| foreach ($variables as $variable) { | |
| $envVariable = Arr::get($variable, 'env_variable'); | |
| $value = Arr::get($variable, 'value'); | |
| /** `@var` EggVariable $eggVariable */ | |
| $eggVariable = $server->egg->variables()->where('env_variable', $envVariable)->first(); | |
| if (!$eggVariable) { | |
| continue; | |
| } | |
| ServerVariable::create([ | |
| 'server_id' => $server->id, | |
| 'variable_id' => $eggVariable->id, | |
| 'variable_value' => $value, | |
| ]); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@app/Services/Servers/Sharing/ServerConfigCreatorService.php` around lines 225
- 239, The importVariables method can dereference a null $eggVariable when
calling ServerVariable::create; add a null-check after retrieving $eggVariable
in importVariables (ServerConfigCreatorService::importVariables) and skip
(continue) creating the ServerVariable if $eggVariable is null, optionally
logging a warning or debug message with the env variable name; mirror the
null-handling approach used in ServerConfigImporterService to avoid the null
pointer dereference when an egg variable is missing.
Comment on lines
+22
to
+29
| if ($file->getError() !== UPLOAD_ERR_OK) { | ||
| throw new InvalidFileUploadException('The selected file was not uploaded successfully'); | ||
| } | ||
|
|
||
| try { | ||
| $parsed = Yaml::parse($file->getContent()); | ||
| } catch (\Exception $exception) { | ||
| throw new InvalidFileUploadException('Could not parse YAML file: ' . $exception->getMessage()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent error message handling - should use translation keys.
The error messages here are hardcoded strings, while ServerConfigCreatorService uses trans() with translation keys. This creates inconsistency and makes localization difficult.
♻️ Proposed fix
public function fromFile(UploadedFile $file, Server $server): void
{
if ($file->getError() !== UPLOAD_ERR_OK) {
- throw new InvalidFileUploadException('The selected file was not uploaded successfully');
+ throw new InvalidFileUploadException(trans('admin/server.import_errors.file_error'));
}
try {
$parsed = Yaml::parse($file->getContent());
} catch (\Exception $exception) {
- throw new InvalidFileUploadException('Could not parse YAML file: ' . $exception->getMessage());
+ throw new InvalidFileUploadException(trans('admin/server.import_errors.parse_error_desc', ['error' => $exception->getMessage()]));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ($file->getError() !== UPLOAD_ERR_OK) { | |
| throw new InvalidFileUploadException('The selected file was not uploaded successfully'); | |
| } | |
| try { | |
| $parsed = Yaml::parse($file->getContent()); | |
| } catch (\Exception $exception) { | |
| throw new InvalidFileUploadException('Could not parse YAML file: ' . $exception->getMessage()); | |
| if ($file->getError() !== UPLOAD_ERR_OK) { | |
| throw new InvalidFileUploadException(trans('admin/server.import_errors.file_error')); | |
| } | |
| try { | |
| $parsed = Yaml::parse($file->getContent()); | |
| } catch (\Exception $exception) { | |
| throw new InvalidFileUploadException(trans('admin/server.import_errors.parse_error_desc', ['error' => $exception->getMessage()])); | |
| } |
🤖 Prompt for AI Agents
In `@app/Services/Servers/Sharing/ServerConfigImporterService.php` around lines 22
- 29, Replace the hardcoded exception messages in ServerConfigImporterService
(the upload error check that throws InvalidFileUploadException and the YAML
parse catch that throws InvalidFileUploadException) with trans() calls using
translation keys consistent with ServerConfigCreatorService; e.g. throw new
InvalidFileUploadException(trans('servers.import.upload_failed')) for the
$file->getError() case and throw new
InvalidFileUploadException(trans('servers.import.parse_failed', ['message' =>
$exception->getMessage()])) in the catch block, and add those keys to the locale
files so localization works the same way as ServerConfigCreatorService.
Comment on lines
+53
to
+65
| if (!$eggUuid) { | ||
| throw new InvalidFileUploadException('Egg UUID is required in the configuration file'); | ||
| } | ||
|
|
||
| $egg = Egg::where('uuid', $eggUuid)->first(); | ||
|
|
||
| if (!$egg) { | ||
| throw new InvalidFileUploadException( | ||
| "Egg with UUID '{$eggUuid}'" . | ||
| ($eggName ? " (name: {$eggName})" : '') . | ||
| ' does not exist in the system' | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use translation keys for error messages.
These error messages should use trans() with the existing translation keys in admin/server.import_errors for consistency with ServerConfigCreatorService.
♻️ Proposed fix
if (!$eggUuid) {
- throw new InvalidFileUploadException('Egg UUID is required in the configuration file');
+ throw new InvalidFileUploadException(trans('admin/server.import_errors.egg_uuid_required'));
}
$egg = Egg::where('uuid', $eggUuid)->first();
if (!$egg) {
- throw new InvalidFileUploadException(
- "Egg with UUID '{$eggUuid}'" .
- ($eggName ? " (name: {$eggName})" : '') .
- ' does not exist in the system'
- );
+ throw new InvalidFileUploadException(
+ trans('admin/server.import_errors.egg_not_found_desc', [
+ 'uuid' => $eggUuid,
+ 'name' => $eggName ?: trans('admin/server.none'),
+ ])
+ );
}🤖 Prompt for AI Agents
In `@app/Services/Servers/Sharing/ServerConfigImporterService.php` around lines 53
- 65, Replace the hard-coded exception messages in ServerConfigImporterService
(the checks around $eggUuid and the $egg lookup) with trans() calls using the
existing admin/server.import_errors keys; throw InvalidFileUploadException with
trans('admin/server.import_errors.egg_uuid_required') when $eggUuid is missing,
and when $egg is not found throw InvalidFileUploadException with
trans('admin/server.import_errors.egg_not_found', ['uuid' => $eggUuid, 'name' =>
$eggName ?? '']) (or omit name if null) so the UUID and optional name are passed
as placeholders to the translation.
Comment on lines
+190
to
+193
| } | ||
|
|
||
| throw new InvalidFileUploadException("Could not find an available port for IP {$ip} starting from port {$startPort}"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use translation key for error message.
This error message should use the translation key for consistency.
♻️ Proposed fix
- throw new InvalidFileUploadException("Could not find an available port for IP {$ip} starting from port {$startPort}");
+ throw new InvalidFileUploadException(trans('admin/server.import_errors.port_exhausted_desc', ['ip' => $ip, 'port' => $startPort]));🤖 Prompt for AI Agents
In `@app/Services/Servers/Sharing/ServerConfigImporterService.php` around lines
190 - 193, Replace the hardcoded error string thrown in
ServerConfigImporterService (the throw new InvalidFileUploadException("Could not
find an available port for IP {$ip} starting from port {$startPort}")) with a
translation lookup using the appropriate translation key and passing the ip and
startPort as parameters (e.g. use trans()/__() with placeholders for ip and
startPort) so the exception message is localized and consistent with other
messages.
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.
This pull request introduces a comprehensive server configuration import/export feature. It allows server configurations to be exported to YAML files (including settings, allocations, and variables) and imported from such files, both via the Filament admin UI and a new API controller.
Import/Export Actions (UI):
ExportServerConfigActionandImportServerConfigActionto the Filament admin UI, enabling users to export a server's configuration as a YAML file and import a server from a YAML file, respectively. These actions include options for customizing what is included in the export and error handling for imports.API Endpoints:
ServerConfigControllerwith endpoints to export a server’s configuration as a downloadable YAML file and to create a server from an uploaded YAML configuration. Includes validation and error handling for both operations.