Add own endpoint for exporting eggs #1760
Conversation
📝 WalkthroughWalkthroughReplaces in-panel egg streaming exports with a route-based export endpoint and adds a controller export action and request validator; adds export route; adds Content-Type header for schedule export; tightens application API authorization and middleware admin check; updates tests and helpers for new admin flag. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Admin Panel (UI)
participant PanelAction as Export Action
participant Router as API Router
participant Req as ExportEggRequest
participant Ctrl as EggController
participant Svc as EggExporterService
participant Resp as StreamedResponse
UI->>PanelAction: User clicks "Export" (select format)
PanelAction->>UI: open/close modal ->close() (modal closed immediately)
PanelAction->>Router: navigate to GET /api/application/eggs/{id}/export?format=...
Router->>Req: validate request
Req-->>Router: validated
Router->>Ctrl: export(request, egg)
rect rgba(230,245,255,0.6)
Ctrl->>Svc: generate export content (egg, format)
Svc-->>Ctrl: stream callback/content
end
Ctrl->>Resp: return StreamedResponse (filename, Content-Type)
Resp-->>UI: file download (streamed)
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 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 (1)
app/Http/Requests/Api/Application/ApplicationApiRequest.php (1)
51-56: Clarify account-key ACL policyThe current fast-path for root admins using TYPE_ACCOUNT still allows non-root admins to pass via
AdminAcl::check(). Decide on the intended policy and implement one of the following:
- Option A (restrict to root admins): for
ApiKey::TYPE_ACCOUNT, only allow whenisRootAdmin()andAdminAcl::check()both pass.- Option B (remove special-case bypass): drop the root-admin fast-path and rely solely on
AdminAcl::check()for all key types.
🧹 Nitpick comments (5)
app/Http/Requests/Api/Application/Eggs/ExportEggRequest.php (1)
7-11: Use enum-based validation to stay in sync with EggFormatReplace the string list with an enum rule to prevent drift and typos. Also confirm a sane default is applied when format is null.
- return [ - 'format' => 'nullable|string|in:yaml,json', - ]; + return [ + 'format' => ['nullable', \Illuminate\Validation\Rule::enum(\App\Enums\EggFormat::class)], + ];Add (outside this hunk):
use App\Enums\EggFormat; use Illuminate\Validation\Rule;app/Http/Requests/Api/Application/ApplicationApiRequest.php (1)
44-48: Verify ability naming and required combo for READUsing “viewList {resource}” AND “view {resource}” for READ is stricter than typical. Confirm these ability names exist and align with User::can() semantics (space vs dot notation) to avoid false 403s on session-based requests. If only viewing a single resource, consider mapping READ to “view {resource}” and letting list endpoints check “viewList {resource}”.
app/Http/Controllers/Api/Application/Eggs/EggController.php (3)
63-64: Use explicit MIME types and add nosniff (recommended).Some clients expect application/x-yaml for YAML. Also add X-Content-Type-Options=nosniff. Optional, but improves compatibility and security.
Apply this diff:
- }, 'egg-' . $egg->getKebabName() . '.' . $format->value, [ - 'Content-Type' => 'application/' . $format->value, - ]); + }, 'egg-' . $egg->getKebabName() . '.' . $format->value, (function () use ($format) { + $contentType = match ($format) { + EggFormat::JSON => 'application/json', + EggFormat::YAML => 'application/x-yaml', + }; + return [ + 'Content-Type' => $contentType, + 'X-Content-Type-Options' => 'nosniff', + ]; + })());
58-58: Optional: support Accept header as a fallback.If no format query is provided, consider deriving format from Accept (application/json vs application/x-yaml) before defaulting to YAML. Keeps it HTTP-native for API consumers.
60-62: Avoid re-querying the egg in the service (micro-optimization).Exporter re-fetches the egg by ID; since you already have the model, consider updating the service to accept Egg (or overload) to drop one DB hit. Low priority.
Based on relevant code snippet for EggExporterService::handle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/Filament/Components/Actions/ExportEggAction.php(1 hunks)app/Filament/Components/Actions/ExportScheduleAction.php(1 hunks)app/Http/Controllers/Api/Application/Eggs/EggController.php(2 hunks)app/Http/Middleware/Api/Application/AuthenticateApplicationUser.php(1 hunks)app/Http/Requests/Api/Application/ApplicationApiRequest.php(1 hunks)app/Http/Requests/Api/Application/Eggs/ExportEggRequest.php(1 hunks)routes/api-application.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
app/Http/Requests/Api/Application/Eggs/ExportEggRequest.php (1)
app/Http/Requests/Api/Application/Eggs/GetEggRequest.php (1)
GetEggRequest(9-14)
routes/api-application.php (1)
app/Http/Controllers/Api/Application/Eggs/EggController.php (1)
EggController(15-66)
app/Http/Controllers/Api/Application/Eggs/EggController.php (4)
app/Http/Controllers/Api/Application/ApplicationApiController.php (1)
ApplicationApiController(14-81)app/Http/Requests/Api/Application/Eggs/ExportEggRequest.php (1)
ExportEggRequest(5-13)app/Models/Egg.php (2)
Egg(60-328)getKebabName(324-327)app/Services/Eggs/Sharing/EggExporterService.php (1)
EggExporterService(12-99)
app/Http/Requests/Api/Application/ApplicationApiRequest.php (3)
app/Services/Acl/Api/AdminAcl.php (1)
AdminAcl(7-39)app/Models/ApiKey.php (2)
user(148-151)ApiKey(56-234)app/Models/User.php (2)
can(358-371)isRootAdmin(380-383)
app/Http/Middleware/Api/Application/AuthenticateApplicationUser.php (1)
app/Models/User.php (1)
isAdmin(385-388)
app/Filament/Components/Actions/ExportEggAction.php (1)
app/Models/Egg.php (1)
Egg(60-328)
🔇 Additional comments (7)
app/Filament/Components/Actions/ExportEggAction.php (1)
43-48: Route-based download + close modal looks good and should avoid the overlay issueOpening in a new tab and closing the modal should resolve #1730’s darkened panel behavior.
Please confirm:
- Response headers set Content-Type and Content-Disposition correctly (per format).
- Filename uses Egg::getKebabName() or equivalent and matches expectations.
- Manual test following issue #1730 steps no longer reproduces the overlay.
app/Http/Middleware/Api/Application/AuthenticateApplicationUser.php (1)
20-22: Unable to locate middleware alias—manual verification needed
Could not find therouteMiddlewareentry forAuthenticateApplicationUserinapp/Http/Kernel.php; please confirm the alias mapping and audit all routes that use this middleware.routes/api-application.php (1)
104-104: New egg export route looks good; confirm middleware/auth behavior.LGTM adding GET /eggs/{egg:id}/export. Please confirm it inherits the same application API middleware and that the new “admin session access” change allows authorized admins (not only root) to hit this route, matching the PR goal.
app/Http/Controllers/Api/Application/Eggs/EggController.php (4)
5-13: Imports updated appropriately.
No issues with the added dependencies.
17-21: Constructor DI is clean.
Injecting EggExporterService via ctor keeps the controller lean.
56-62: Export endpoint implementation is correct and aligns with the new route.
Validation via ExportEggRequest + EggFormat fallback to YAML is solid.
56-65: Verify UI regression fix (panel overlay) end-to-end.This route-based download should prevent the Filament action overlay. Please confirm the front-end now triggers a direct file download without leaving the page darkened, per issue #1730.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Traits/Http/RequestMockHelpers.php (1)
36-44: Align the docblock with the method signatureLine 36 still advertises that
generateRequestUserModel“also returns the generated model,” but the method now returnsvoid. To avoid confusion for future tests that might rely on the doc, please update the docblock (or reintroduce the return). For example:- /** - * Generates a new request user model and also returns the generated model. - */ + /** + * Generates a new request user model and wires it into the mocked request. + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/Traits/Http/RequestMockHelpers.php(1 hunks)tests/Unit/Http/Middleware/Api/Application/AuthenticateUserTest.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/Traits/Http/RequestMockHelpers.php (2)
app/Models/User.php (3)
isAdmin(385-388)isRootAdmin(380-383)User(95-493)app/Models/Role.php (1)
isRootAdmin(124-127)
tests/Unit/Http/Middleware/Api/Application/AuthenticateUserTest.php (3)
tests/Traits/Http/RequestMockHelpers.php (1)
generateRequestUserModel(38-47)app/Http/Middleware/Api/Application/AuthenticateApplicationUser.php (1)
handle(16-25)tests/Traits/Http/MocksMiddlewareClosure.php (1)
getClosureAssertions(14-24)
🔇 Additional comments (1)
tests/Unit/Http/Middleware/Api/Application/AuthenticateUserTest.php (1)
48-52: Nice coverage for the root-admin pathLine 48 ensures we exercise the session-authenticated root admin case, so we have explicit protection against regressions when tightening the middleware earlier in the stack. 👍
Closes #1730
Adds dedicated endpoint for exporting eggs as file. Also changes the auth for the application api so admins can access the api with their session. (which previously was exclusive only for the root admin)