feat: async file post-processing pipeline for company logos#561
Open
smarcet wants to merge 5 commits into
Open
feat: async file post-processing pipeline for company logos#561smarcet wants to merge 5 commits into
smarcet wants to merge 5 commits into
Conversation
- Add FileInfoDTO (owner entity class/id/member, filepath, md5, size, mime_type, source_bucket) - Add FileProcessingJob (ShouldQueue) dispatching to FilePostProcessorService - Add FilePostProcessorService with locateService() switch dispatch; throws InvalidArgumentException for unknown entity classes instead of silent false - Add IFilePostProcessorService / IFilePostProcessorForChildEntity interfaces - Add CompanyService::processFileForChildEntity() handling 'logo' and 'big_logo' members: downloads from remote storage, verifies size and MD5, builds UploadedFile, cleans up temp files in finally block - Fix md5_file() false detection: I/O failure and hash mismatch now produce distinct error messages - Use Company::LogoAllowedExtensions constant in addCompanyLogo/addCompanyBigLogo (removed local copies) - Update OAuth2CompaniesApiController: addCompanyLogo/addCompanyBigLogo now accept both multipart/form-data (file upload) and application/json (async file-api payload); intval() cast on company_id - Update OpenAPI specs for both endpoints: dual RequestBody with multipart and JSON MediaType entries; 412 removed; description updated - Fix CompanyValidationRulesFactory::buildForFileInfo() signature (remove unused $data param) - Remove unused Illuminate\Support\Facades\Request import from controller - Register FilePostProcessorService in AppServiceProvider and ModelServicesProvider Tests: - Add CompanyFileProcessingTest (5 unit tests): unknown member name, MD5 mismatch vs I/O error distinction, big_logo mismatch, unknown entity class throws, FileInfoDTO queue serialization round-trip - Add OAuth2CompaniesApiTest dual-flow functional tests (6 tests): logo multipart, logo JSON payload, logo JSON missing fields -> 412, big_logo multipart, big_logo JSON payload, backward-compat shim
|
Warning Review limit reached
More reviews will be available in 32 minutes and 7 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR adds a second company logo upload path: in addition to multipart file uploads, endpoints now accept JSON payloads referencing files already stored in a file API. A new ChangesCompany Logo Dual-Path Upload
Sequence Diagram(s)sequenceDiagram
participant Client
participant OAuth2CompaniesApiController
participant CompanyService
participant FilePostProcessorService
participant FileProcessingJob
participant FileUtils
rect rgba(100, 149, 237, 0.5)
Note over Client,CompanyService: Multipart upload path
Client->>OAuth2CompaniesApiController: POST /companies/{id}/logo (multipart file)
OAuth2CompaniesApiController->>CompanyService: addCompanyLogo(company_id, FileUploadInfo)
CompanyService-->>OAuth2CompaniesApiController: IEntity
OAuth2CompaniesApiController-->>Client: 201 JSON
end
rect rgba(144, 238, 144, 0.5)
Note over Client,FileUtils: JSON file-API payload path
Client->>OAuth2CompaniesApiController: POST /companies/{id}/logo (JSON filepath/filename/md5/size)
OAuth2CompaniesApiController->>CompanyService: processFileForChildEntity(FileInfoDTO{owner_member_name=logo})
CompanyService->>FileUtils: getFileFromRemoteStorageOnTempStorage(filepath)
FileUtils-->>CompanyService: local temp path
CompanyService->>CompanyService: verify MD5, create UploadedFile
CompanyService->>CompanyService: addCompanyLogo(company_id, UploadedFile)
CompanyService->>FileUtils: cleanLocalAndRemoteFile(...)
CompanyService-->>OAuth2CompaniesApiController: IEntity
OAuth2CompaniesApiController-->>Client: 201 JSON
end
rect rgba(255, 165, 100, 0.5)
Note over CompanyService,FileProcessingJob: addCompany async logo dispatch
CompanyService->>FileProcessingJob: dispatch(FileInfoDTO) via JobDispatcher
FileProcessingJob->>FilePostProcessorService: postProcessFileFromFileApi(FileInfoDTO)
FilePostProcessorService->>FilePostProcessorService: locateService(Company::class)
FilePostProcessorService->>CompanyService: processFileForChildEntity(FileInfoDTO)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-561/ This page is automatically updated on each push to this PR. |
…t schemas - Add reusable FileDTO schema with filepath, filename, md5, size (required) and mime_type, source_bucket (optional) - Add optional logo and big_logo properties (ref FileDTO) to CompanyCreateRequest and CompanyUpdateRequest schemas
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-561/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Http/Utils/FileUploadInfo.php`:
- Around line 142-167: In the buildFromPayload method, use the normalized
trimmed path variable consistently and always retrieve the actual file size from
storage. Replace the untrimmed $payload['filepath'] with the trimmed $filepath
variable in the $disk->exists() call on line 147, and all subsequent references
including the logging on line 155 and the disk->path() call on line 161.
Additionally, remove the conditional check that allows trusting the
client-provided $payload['size'] value on line 153 and instead always retrieve
the actual size from the disk using $disk->size($filepath), ensuring the
zero-size validation guard cannot be bypassed by forged client metadata.
In `@app/Jobs/FileProcessingJob.php`:
- Around line 36-38: The handle method in FileProcessingJob is not checking the
return value from the postProcessFileFromFileApi call on the injected
IFilePostProcessorService. When the service method returns false, the job
currently completes successfully instead of failing and retrying. Capture the
return value from the postProcessFileFromFileApi call with $this->fileInfoDTO as
the argument, check if it returns false, and throw an exception in that case to
properly fail the job and trigger retries.
In `@app/Services/Model/FileInfoDTO.php`:
- Around line 28-30: The __toString() method in the FileInfoDTO class currently
calls json_encode() directly, but json_encode() can return false on encoding
errors, which triggers a TypeError when returning from a method declared to
return string. Wrap the json_encode call with a null check or ternary operator
to handle the false case, providing a fallback string value (such as an empty
object representation or error message) to ensure the method always returns a
valid string instead of false.
In `@app/Services/Model/Imp/CompanyService.php`:
- Around line 306-308: The finally block in the CompanyService cleanup logic
unconditionally deletes both local and remote files using
cleanLocalAndRemoteFile, which destroys the remote source file even when
processing fails, preventing retries. Modify the logic so that the remote file
(referenced in file_info_dto->filepath) is only deleted on successful processing
completion, while the local file can still be cleaned up in the finally block.
Apply this fix to both occurrences of the finally block that call
cleanLocalAndRemoteFile (around lines 306-308 and 332-334).
In `@tests/oauth2/OAuth2CompaniesApiTest.php`:
- Around line 108-124: The testAddCompanyLogoViaJsonPayload method and other
JSON-flow tests are faking storage disks but still depend on the
file_upload.storage_driver configuration resolving to 'local', making them
vulnerable to environment or config drift. Explicitly pin the storage driver
configuration to 'local' at the beginning of each affected JSON test method
(including testAddCompanyLogoViaJsonPayload and the test at lines 203-219) using
Laravel's Config facade to set the driver value explicitly, ensuring the tests
use the faked 'local' disk regardless of environment settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16d1129f-3e6d-4bfa-8e10-d74fe3630acf
📒 Files selected for processing (16)
Libs/Utils/FileUtils.phpapp/Http/Controllers/Apis/Protected/Main/Factories/CompanyValidationRulesFactory.phpapp/Http/Controllers/Apis/Protected/Main/OAuth2CompaniesApiController.phpapp/Http/Utils/FileUploadInfo.phpapp/Jobs/FileProcessingJob.phpapp/Models/Foundation/Main/Companies/Company.phpapp/Providers/AppServiceProvider.phpapp/Services/FilePostProcessorService.phpapp/Services/Model/FileInfoDTO.phpapp/Services/Model/ICompanyService.phpapp/Services/Model/IFilePostProcessorForChildEntity.phpapp/Services/Model/IFilePostProcessorService.phpapp/Services/Model/Imp/CompanyService.phpapp/Services/ModelServicesProvider.phptests/Unit/Services/CompanyFileProcessingTest.phptests/oauth2/OAuth2CompaniesApiTest.php
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-561/ This page is automatically updated on each push to this PR. |
- FileUploadInfo::buildFromPayload: use trimmed $filepath consistently in exists(),
log, and path() calls; always get file size from disk (never trust client payload)
- FileProcessingJob::handle: throw RuntimeException when postProcessFileFromFileApi
returns false so the job fails and triggers retries instead of completing silently
- FileInfoDTO::__toString: guard json_encode false return with ?: '{}' fallback
- CompanyService::processFileForChildEntity: use success flag so remote source file
is only deleted on success; on failure only local temp is cleaned up, preserving
the remote file for queue job retries (both logo and big_logo cases)
- FileUtils: add cleanLocalFile() helper for local-only cleanup
- OAuth2CompaniesApiTest: pin file_upload.storage_driver to 'local' in JSON-path
tests so they are immune to environment/config drift
- FileProcessingJob: add backoff() [30s, 60s] so retries don't fire back-to-back against a throttled S3 endpoint - FileUtils: preserve original exception chain in ValidationException re-throw so upstream handlers and job failure logs retain the root cause and stack trace
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-561/ This page is automatically updated on each push to this PR. |
1 similar comment
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-561/ This page is automatically updated on each push to this PR. |
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.
ref:https://app.clickup.com/t/9014802374/86bah99tf
Tests:
Summary by CodeRabbit
New Features
Tests