Egg API Import/Delete #1947
Egg API Import/Delete #1947
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
📝 WalkthroughWalkthroughThis PR adds egg import and delete functionality to the application's API. It introduces a new Changes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/Services/Eggs/Sharing/EggImporterService.php (2)
35-98: fromContent/fromFile factoring is good; tighten handling of$parsed['variables']The new
fromContent()+fromParsed()split nicely separates transport (string/file/url) from the core import logic, which makes the new API endpoint straightforward to use.One edge case in
fromParsed():for ($i = 0; $i < count($parsed['variables']); $i++) { unset($parsed['variables'][$i]['field_type']); }If the parsed egg has no
variableskey or it’s not an array,count($parsed['variables'])will emit warnings. Since later code already guards with?? []in some places, you could make this loop more defensive:- for ($i = 0; $i < count($parsed['variables']); $i++) { - unset($parsed['variables'][$i]['field_type']); - } + $variables = $parsed['variables'] ?? []; + + foreach ($variables as $i => $variable) { + unset($variables[$i]['field_type']); + } + + $parsed['variables'] = $variables;This keeps behavior the same for normal inputs while avoiding noisy warnings on minimal/edge-case egg definitions.
128-235: UseUploadedFile::get()instead ofgetContent()— this is a critical correctness issueThe current code calls
$file->getContent()in theparseFile()method, butIlluminate\Http\UploadedFiledoes not expose agetContent()method. The correct public API method isget(), which returns the file contents as a string.Using
getContent()will fail at runtime. The Laravel UploadedFile implementation wraps Symfony's File class, which internally usesgetContent(), but it is not part of Laravel's public API for UploadedFile.- $content = $file->getContent(); + $content = $file->get();Additionally, the current error handling wraps parse errors twice:
parse()already catches errors and throwsInvalidFileUploadException('File parse failed: ...'), and thenparseFile()catches and re-wraps them. This can produce nested messages like"File parse failed: File parse failed: ...". To avoid this:} catch (Throwable $e) { - throw new InvalidFileUploadException('File parse failed: ' . $e->getMessage()); + if ($e instanceof InvalidFileUploadException) { + throw $e; + } + + throw new InvalidFileUploadException('File parse failed: ' . $e->getMessage()); }
🧹 Nitpick comments (2)
routes/api-application.php (1)
101-108: Duplicate route name for two DELETE egg endpointsBoth
DELETE /eggs/{egg:id}andDELETE /eggs/{egg:uuid}/uuidshare the same route name (api.application.eggs.eggs.delete). Laravel will let both routes exist, butroute('api.application.eggs.eggs.delete', ...)will only ever point to the last-defined one.If you plan to generate URLs for both forms, consider giving them distinct names (e.g.,
.deletevs.delete_uuid). Also, you may want to drop the duplicatedeggssegment in the name (api.application.eggs.delete) for consistency with other groups.app/Http/Controllers/Api/Application/Eggs/EggController.php (1)
58-70: delete() implementation is fine; unused request is intentionalThe delete endpoint cleanly delegates to
$egg->delete()and returns a 204 viareturnNoContent(). The$requestparameter is unused in the method body, but it’s still useful to keep for validation/authorization tied toGetEggRequest. If PHPMD complains, I’d treat that as a false positive rather than removing the type-hinted request.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/Http/Controllers/Api/Application/Eggs/EggController.php(3 hunks)app/Http/Requests/Api/Application/Eggs/ImportEggRequest.php(1 hunks)app/Http/Requests/Api/Application/Eggs/PostEggRequest.php(1 hunks)app/Services/Eggs/Sharing/EggImporterService.php(5 hunks)routes/api-application.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
app/Http/Requests/Api/Application/Eggs/ImportEggRequest.php (1)
app/Http/Requests/Api/Application/Eggs/PostEggRequest.php (1)
PostEggRequest(9-14)
routes/api-application.php (1)
app/Http/Controllers/Api/Application/Eggs/EggController.php (2)
delete(65-70)EggController(21-120)
app/Http/Controllers/Api/Application/Eggs/EggController.php (3)
app/Exceptions/Service/InvalidFileUploadException.php (1)
InvalidFileUploadException(7-7)app/Http/Requests/Api/Application/Eggs/ImportEggRequest.php (1)
ImportEggRequest(5-13)app/Services/Eggs/Sharing/EggImporterService.php (3)
EggImporterService(20-300)__construct(33-33)fromContent(40-45)
app/Services/Eggs/Sharing/EggImporterService.php (1)
app/Exceptions/Service/InvalidFileUploadException.php (1)
InvalidFileUploadException(7-7)
🪛 PHPMD (2.15.0)
app/Http/Controllers/Api/Application/Eggs/EggController.php
65-65: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (2)
app/Http/Requests/Api/Application/Eggs/PostEggRequest.php (1)
5-14: PostEggRequest wiring looks consistentBinding the resource to
Egg::RESOURCE_NAMEand permission toAdminAcl::WRITEmatches the intended write-only semantics for POST-style egg operations. No issues from a correctness perspective.app/Http/Requests/Api/Application/Eggs/ImportEggRequest.php (1)
5-12: Import format validation is solidRestricting
formattoyaml|jsonand making it nullable aligns well withEggFormat::tryFrom(... ) ?? EggFormat::YAMLin the controller. This gives a clear contract for callers and keeps the enum mapping straightforward.
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/Http/Controllers/Api/Application/Eggs/EggController.php(3 hunks)app/Services/Eggs/Sharing/EggImporterService.php(5 hunks)routes/api-application.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T19:40:12.104Z
Learnt from: rmartinoscar
Repo: pelican-dev/panel PR: 1671
File: app/Filament/Admin/Resources/ServerResource/Pages/EditServer.php:690-700
Timestamp: 2025-09-05T19:40:12.104Z
Learning: In admin panel contexts (app/Filament/Admin/Resources/), it's acceptable to show raw exception messages ($e->getMessage()) to admin users for debugging purposes, as they need detailed error information.
Applied to files:
app/Http/Controllers/Api/Application/Eggs/EggController.php
🧬 Code graph analysis (2)
routes/api-application.php (1)
app/Http/Controllers/Api/Application/Eggs/EggController.php (2)
delete(65-70)EggController(21-119)
app/Services/Eggs/Sharing/EggImporterService.php (1)
app/Exceptions/Service/InvalidFileUploadException.php (1)
InvalidFileUploadException(7-7)
🪛 PHPMD (2.15.0)
app/Http/Controllers/Api/Application/Eggs/EggController.php
65-65: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (6)
routes/api-application.php (1)
104-107: LGTM!The new egg routes are well-structured and consistent with existing patterns in the file. Providing both numeric ID and UUID-based delete endpoints offers good flexibility for API consumers.
app/Http/Controllers/Api/Application/Eggs/EggController.php (1)
58-70: LGTM!The delete method is clean and follows Laravel conventions. The
GetEggRequestparameter handles authorization even though it's not explicitly used in the method body—this is a standard Laravel pattern.app/Services/Eggs/Sharing/EggImporterService.php (4)
35-45: LGTM!The new
fromContentmethod provides a clean entry point for content-based egg imports. The separation of parsing (parse) and processing (fromParsed) follows good design principles and enables reuse across different import sources.
52-100: LGTM!The refactoring to extract
fromParsedis excellent—it centralizes the transaction logic and makes it reusable across all import methods (fromContent,fromFile,fromUrl). This design follows the single responsibility principle and improves testability.
137-207: LGTM!The refactored
parsemethod effectively decouples parsing logic from file handling. The use of amatchexpression for format selection is clean and type-safe. Error wrapping withInvalidFileUploadExceptionprovides consistent exception handling for API responses.
209-236: LGTM!The new
parseFilemethod properly handles uploaded files with appropriate error checking. The format detection logic using both file extension and MIME type is robust, and delegating to theparsemethod maintains good separation of concerns.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/Http/Controllers/Api/Application/Eggs/EggController.php (1)
88-118: Don’t expose internal exception messages in the 500 response; log insteadThe overall import flow (detect
EggFormat, read raw body, delegate toEggImporterService::fromContent, and return a 201 with the transformed egg) looks solid, and mappingInvalidFileUploadExceptionto a 422 is reasonable.However, the generic
catch (\Throwable $e)block still concatenates$e->getMessage()into the client-facingmessagefield, which can leak internal details and was already flagged in a previous review. It also swallows the exception without logging/reporting it.Consider either letting the exception bubble to Laravel’s handler, or updating the block along these lines:
- } catch (\Throwable $e) { - return response()->json([ - 'error' => 'Unable to import egg', - 'message' => 'An unexpected error occurred. ' . $e->getMessage(), - ], 500); - } + } catch (\Throwable $e) { + report($e); + + return response()->json([ + 'error' => 'Unable to import egg', + 'message' => 'An unexpected error occurred while importing the egg.', + ], 500); + }This avoids leaking internal messages while still returning a stable 500 payload and ensures the exception is logged.
🧹 Nitpick comments (1)
app/Http/Controllers/Api/Application/Eggs/EggController.php (1)
58-70: Delete implementation is fine; keep the unused FormRequest parameterThe delete logic (
$egg->delete()thenreturnNoContent()) is straightforward and appropriate.PHPMD flags
$requestas unused, but in this pattern theGetEggRequestparameter is typically there to trigger itsauthorize()/rules()side effects (auth/validation) even if the variable is not referenced. I’d keep the parameter and, if the PHPMD warning is noisy, consider suppressing it at the rule or method level instead of removing the parameter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Http/Controllers/Api/Application/Eggs/EggController.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Http/Controllers/Api/Application/Eggs/EggController.php (3)
app/Exceptions/Service/InvalidFileUploadException.php (1)
InvalidFileUploadException(7-7)app/Http/Requests/Api/Application/Eggs/ImportEggRequest.php (1)
ImportEggRequest(5-13)app/Services/Eggs/Sharing/EggImporterService.php (2)
EggImporterService(20-301)fromContent(40-45)
🪛 PHPMD (2.15.0)
app/Http/Controllers/Api/Application/Eggs/EggController.php
65-65: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (1)
app/Http/Controllers/Api/Application/Eggs/EggController.php (1)
5-26: Constructor wiring and new imports look consistentInjecting
EggImporterServiceand adding the corresponding imports (ImportEggRequest,InvalidFileUploadException,JsonResponse,Response) aligns with the new import endpoint usage; types are consistent and there’s no obvious wiring issue.
Co-authored-by: Boy132 <Boy132@users.noreply.github.com>
combined ImportEggRequest.php and PostEggRequest.php
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Services/Eggs/Sharing/EggImporterService.php (1)
74-93: Consider wrapping HTTP exceptions for consistency.The
fromFilemethod wraps all exceptions inInvalidFileUploadException, butfromUrlallows HTTP exceptions (from->throw()on line 90) to bubble up unwrapped. While not incorrect, this creates a minor inconsistency in the error handling approach.Apply this diff to align error handling:
public function fromUrl(string $url, ?Egg $egg = null): Egg { $info = pathinfo($url); $extension = strtolower($info['extension']); $format = match ($extension) { 'yaml', 'yml' => EggFormat::YAML, 'json' => EggFormat::JSON, default => throw new InvalidFileUploadException('Unsupported file format.'), }; - $content = Http::timeout(5)->connectTimeout(1)->get($url)->throw()->body(); - - return $this->fromContent($content, $format, $egg); + try { + $content = Http::timeout(5)->connectTimeout(1)->get($url)->throw()->body(); + + return $this->fromContent($content, $format, $egg); + } catch (Throwable $e) { + throw new InvalidFileUploadException('Failed to fetch egg from URL: ' . $e->getMessage()); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/Http/Controllers/Api/Application/Eggs/EggController.php(3 hunks)app/Http/Requests/Api/Application/Eggs/ImportEggRequest.php(1 hunks)app/Services/Eggs/Sharing/EggImporterService.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Http/Requests/Api/Application/Eggs/ImportEggRequest.php
🧰 Additional context used
🧬 Code graph analysis (1)
app/Http/Controllers/Api/Application/Eggs/EggController.php (2)
app/Http/Requests/Api/Application/Eggs/ImportEggRequest.php (1)
ImportEggRequest(9-14)app/Services/Eggs/Sharing/EggImporterService.php (2)
EggImporterService(20-279)fromContent(40-45)
🪛 PHPMD (2.15.0)
app/Http/Controllers/Api/Application/Eggs/EggController.php
65-65: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (7)
app/Http/Controllers/Api/Application/Eggs/EggController.php (3)
10-10: LGTM! Imports are appropriate.The new imports for
ImportEggRequest,EggImporterService,Exception,JsonResponse,Response, andThrowableare correctly added to support the import and delete functionality.Also applies to: 13-13, 15-19
23-28: LGTM! Clean dependency injection.The
EggImporterServiceis properly injected via constructor, following Laravel's dependency injection patterns.
58-70: No action needed — egg deletion is safely protected by model validation.The Egg model's
deletingevent handler (lines 162-166 in app/Models/Egg.php) prevents deletion if the egg has associated servers or child eggs, throwingHasActiveServersExceptionbefore deletion occurs. Additionally, the foreign key constraint onservers.egg_idis configured without cascading deletes, providing database-level protection. The UI also disables the delete button for eggs in use (Filament UI, line 453).The
$requestparameter is correctly used for authorization through route model binding.Likely an incorrect or invalid review comment.
app/Services/Eggs/Sharing/EggImporterService.php (4)
35-45: LGTM! Clean entry point for content-based import.The
fromContentmethod provides a clear public API for importing eggs from string content with configurable format. The delegation toparseandfromParsedfollows good separation of concerns.
52-72: LGTM! Robust file upload handling.The method properly validates upload success, determines format from both extension and MIME type, and wraps parsing errors in
InvalidFileUploadExceptionwith helpful context. The fallback to JSON when YAML is not detected is a reasonable default.
95-136: LGTM! Solid transactional import logic.The
fromParsedmethod correctly handles egg creation/updates within a database transaction, manages UUID generation and lookup, processes variables with flexible rule handling, and cleans up variables that are no longer in the import. The visibility change toprotectedis appropriate for the refactored architecture.
138-214: LGTM! Comprehensive parsing with excellent backward compatibility.The
parsemethod handles multiple egg format versions (PTDL_v1, PTDL_v2, PLCN_v1/v2, PLCN_v3), normalizes nested config and script structures to JSON, and intelligently renames reserved environment variables by prefixing them withSERVER_. The upgrade variable substitution (lines 173-178) ensures legacy configuration references are updated. Error handling provides helpful context for debugging.
Boy132
left a comment
Boy132
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
* nullish? * Fix reading files contents axios was deserializing json automatically before * Fix serverfile "no root" error root is unset when dir is set but empty * autogenerated types * nodes allocs support pagination * Docs [1/X] * fix states for server types * Implement pelican-dev/panel#1947 * Closes pelican-dev/panel#1947 * DOCS done Im lazy to do more
3 participants
Added an endpoint for importing and deleting eggs via the api.
This is the first time I'm working with PHP and any criticism is welcome. If anything needs to be changed let me know. 😄