From 476846ca7b9372b32f208cfdfa93764d7efb4f81 Mon Sep 17 00:00:00 2001 From: smarcet Date: Fri, 19 Jun 2026 17:31:09 -0300 Subject: [PATCH 1/5] feat: async file post-processing pipeline for company logos - 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 --- Libs/Utils/FileUtils.php | 5 +- .../CompanyValidationRulesFactory.php | 16 +- .../Main/OAuth2CompaniesApiController.php | 139 +++++++--- app/Http/Utils/FileUploadInfo.php | 94 +++++-- app/Jobs/FileProcessingJob.php | 44 ++++ .../Foundation/Main/Companies/Company.php | 3 +- app/Providers/AppServiceProvider.php | 27 ++ app/Services/FilePostProcessorService.php | 49 ++++ app/Services/Model/FileInfoDTO.php | 31 +++ app/Services/Model/ICompanyService.php | 4 +- .../IFilePostProcessorForChildEntity.php | 16 ++ .../Model/IFilePostProcessorService.php | 19 ++ app/Services/Model/Imp/CompanyService.php | 146 ++++++++++- app/Services/ModelServicesProvider.php | 12 +- .../Services/CompanyFileProcessingTest.php | 240 ++++++++++++++++++ tests/oauth2/OAuth2CompaniesApiTest.php | 173 ++++++++++++- 16 files changed, 941 insertions(+), 77 deletions(-) create mode 100644 app/Jobs/FileProcessingJob.php create mode 100644 app/Services/FilePostProcessorService.php create mode 100644 app/Services/Model/FileInfoDTO.php create mode 100644 app/Services/Model/IFilePostProcessorForChildEntity.php create mode 100644 app/Services/Model/IFilePostProcessorService.php create mode 100644 tests/Unit/Services/CompanyFileProcessingTest.php diff --git a/Libs/Utils/FileUtils.php b/Libs/Utils/FileUtils.php index 675e436c9..cc614bc27 100644 --- a/Libs/Utils/FileUtils.php +++ b/Libs/Utils/FileUtils.php @@ -62,7 +62,10 @@ public static function getFileFromRemoteStorageOnTempStorage(string $file_name, stream_set_blocking($stream, true); @stream_set_read_buffer($stream, 0); - $localPath = rtrim(self::getLocalTmpStorage(), '/')."/".$file_name; + $localPath = tempnam(self::getLocalTmpStorage(), 'fproc_'); + if ($localPath === false) { + throw new ValidationException("Unable to create local temporary file."); + } $out = fopen($localPath, 'wb'); // binary mode if ($out === false) { diff --git a/app/Http/Controllers/Apis/Protected/Main/Factories/CompanyValidationRulesFactory.php b/app/Http/Controllers/Apis/Protected/Main/Factories/CompanyValidationRulesFactory.php index 7dcad4622..bb6d1c5b9 100644 --- a/app/Http/Controllers/Apis/Protected/Main/Factories/CompanyValidationRulesFactory.php +++ b/app/Http/Controllers/Apis/Protected/Main/Factories/CompanyValidationRulesFactory.php @@ -69,6 +69,20 @@ public static function build(array $data, $update = false) 'overview' => 'nullable|string', 'commitment' => 'nullable|string', 'commitment_author' => 'nullable|string', + 'big_logo' => 'sometimes|file_dto', + 'logo' =>'sometimes|file_dto', ]; } -} \ No newline at end of file + + + public static function buildForFileInfo(): array { + return [ + 'filepath' => 'required|string', + 'filename' => 'required|string', + 'md5' => 'required|string', + 'size' => 'required|integer', + 'mime_type'=> 'sometimes|string', + 'source_bucket' => 'sometimes|string', + ]; + } +} diff --git a/app/Http/Controllers/Apis/Protected/Main/OAuth2CompaniesApiController.php b/app/Http/Controllers/Apis/Protected/Main/OAuth2CompaniesApiController.php index 1a2ab7758..fab0eec11 100644 --- a/app/Http/Controllers/Apis/Protected/Main/OAuth2CompaniesApiController.php +++ b/app/Http/Controllers/Apis/Protected/Main/OAuth2CompaniesApiController.php @@ -19,10 +19,12 @@ use App\Rules\Boolean; use App\Security\CompanyScopes; use App\Security\SummitScopes; +use App\Services\Model\FileInfoDTO; use App\Services\Model\ICompanyService; use Illuminate\Http\Request as LaravelRequest; use Illuminate\Http\Response; use models\exceptions\ValidationException; +use models\main\Company; use models\main\ICompanyRepository; use models\oauth2\IResourceServerContext; use models\utils\IEntity; @@ -505,7 +507,7 @@ protected function updateEntity($id, array $payload): IEntity path: "/api/v1/companies/{id}/logo", operationId: "addCompanyLogo", summary: "Add company logo", - description: "Uploads a logo image for the company", + description: "Uploads a logo image for the company. Accepts either a multipart file upload or a JSON payload referencing a file already stored in the file API.", security: [ [ "companies_oauth2" => [ @@ -531,20 +533,36 @@ protected function updateEntity($id, array $payload): IEntity ], requestBody: new OA\RequestBody( required: true, - content: new OA\MediaType( - mediaType: "multipart/form-data", - schema: new OA\Schema( - required: ["file"], - properties: [ - new OA\Property( - property: "file", - type: "string", - format: "binary", - description: "Logo image file" - ) - ] - ) - ) + content: [ + new OA\MediaType( + mediaType: "multipart/form-data", + schema: new OA\Schema( + required: ["file"], + properties: [ + new OA\Property( + property: "file", + type: "string", + format: "binary", + description: "Logo image file" + ) + ] + ) + ), + new OA\MediaType( + mediaType: "application/json", + schema: new OA\Schema( + required: ["filepath", "filename", "md5", "size"], + properties: [ + new OA\Property(property: "filepath", type: "string", description: "Remote storage path of the uploaded file"), + new OA\Property(property: "filename", type: "string", description: "Original file name"), + new OA\Property(property: "md5", type: "string", description: "MD5 hash of the file for integrity verification"), + new OA\Property(property: "size", type: "integer", description: "File size in bytes"), + new OA\Property(property: "mime_type", type: "string", description: "MIME type of the file"), + new OA\Property(property: "source_bucket", type: "string", description: "Source storage bucket"), + ] + ) + ), + ] ), responses: [ new OA\Response( @@ -556,7 +574,6 @@ protected function updateEntity($id, array $payload): IEntity new OA\Response(response: Response::HTTP_UNAUTHORIZED, description: "Unauthorized"), new OA\Response(response: Response::HTTP_FORBIDDEN, description: "Forbidden"), new OA\Response(response: Response::HTTP_NOT_FOUND, description: "Company not found"), - new OA\Response(response: Response::HTTP_PRECONDITION_FAILED, description: "File parameter not set"), ] )] public function addCompanyLogo(LaravelRequest $request, $company_id) @@ -564,12 +581,26 @@ public function addCompanyLogo(LaravelRequest $request, $company_id) return $this->processRequest(function () use ($request, $company_id) { $file = $request->file('file'); - if (is_null($file)) { - return $this->error412(array('file param not set!')); - } + if (!is_null($file)) { + $logo = $this->service->addCompanyLogo(intval($company_id), $file); - $logo = $this->service->addCompanyLogo(intval($company_id), $file); + return $this->created(SerializerRegistry::getInstance()->getSerializer($logo)->serialize()); + } + // file api post processing ( new flow ) + $payload = $this->getJsonPayload(CompanyValidationRulesFactory::buildForFileInfo()); + + $logo = $this->service->processFileForChildEntity(new FileInfoDTO( + owner_entity_id: intval($company_id), + owner_entity_class: Company::class, + owner_member_name: 'logo', + filepath: $payload['filepath'], + filename: $payload['filename'], + size: intval($payload['size']), + md5: $payload['md5'] ?? null, + mime_type: $payload['mime_type'] ?? null, + source_bucket: $payload['source_bucket'] ?? null, + )); return $this->created(SerializerRegistry::getInstance()->getSerializer($logo)->serialize()); }); } @@ -624,7 +655,7 @@ public function deleteCompanyLogo($company_id) path: "/api/v1/companies/{id}/logo/big", operationId: "addCompanyBigLogo", summary: "Add company big logo", - description: "Uploads a big logo image for the company", + description: "Uploads a big logo image for the company. Accepts either a multipart file upload or a JSON payload referencing a file already stored in the file API.", security: [ [ "companies_oauth2" => [ @@ -650,20 +681,36 @@ public function deleteCompanyLogo($company_id) ], requestBody: new OA\RequestBody( required: true, - content: new OA\MediaType( - mediaType: "multipart/form-data", - schema: new OA\Schema( - required: ["file"], - properties: [ - new OA\Property( - property: "file", - type: "string", - format: "binary", - description: "Big logo image file" - ) - ] - ) - ) + content: [ + new OA\MediaType( + mediaType: "multipart/form-data", + schema: new OA\Schema( + required: ["file"], + properties: [ + new OA\Property( + property: "file", + type: "string", + format: "binary", + description: "Big logo image file" + ) + ] + ) + ), + new OA\MediaType( + mediaType: "application/json", + schema: new OA\Schema( + required: ["filepath", "filename", "md5", "size"], + properties: [ + new OA\Property(property: "filepath", type: "string", description: "Remote storage path of the uploaded file"), + new OA\Property(property: "filename", type: "string", description: "Original file name"), + new OA\Property(property: "md5", type: "string", description: "MD5 hash of the file for integrity verification"), + new OA\Property(property: "size", type: "integer", description: "File size in bytes"), + new OA\Property(property: "mime_type", type: "string", description: "MIME type of the file"), + new OA\Property(property: "source_bucket", type: "string", description: "Source storage bucket"), + ] + ) + ), + ] ), responses: [ new OA\Response( @@ -675,19 +722,31 @@ public function deleteCompanyLogo($company_id) new OA\Response(response: Response::HTTP_UNAUTHORIZED, description: "Unauthorized"), new OA\Response(response: Response::HTTP_FORBIDDEN, description: "Forbidden"), new OA\Response(response: Response::HTTP_NOT_FOUND, description: "Company not found"), - new OA\Response(response: Response::HTTP_PRECONDITION_FAILED, description: "File parameter not set"), ] )] public function addCompanyBigLogo(LaravelRequest $request, $company_id) { return $this->processRequest(function () use ($request, $company_id) { $file = $request->file('file'); - if (is_null($file)) { - return $this->error412(array('file param not set!')); + if (!is_null($file)) { + $logo = $this->service->addCompanyBigLogo(intval($company_id), $file); + return $this->created(SerializerRegistry::getInstance()->getSerializer($logo)->serialize()); } - $logo = $this->service->addCompanyBigLogo(intval($company_id), $file); - + // file api post processing ( new flow ) + $payload = $this->getJsonPayload(CompanyValidationRulesFactory::buildForFileInfo()); + + $logo = $this->service->processFileForChildEntity(new FileInfoDTO( + owner_entity_id: intval($company_id), + owner_entity_class: Company::class, + owner_member_name: 'big_logo', + filepath: $payload['filepath'], + filename: $payload['filename'], + size: intval($payload['size']), + md5: $payload['md5'] ?? null, + mime_type: $payload['mime_type'] ?? null, + source_bucket: $payload['source_bucket'] ?? null, + )); return $this->created(SerializerRegistry::getInstance()->getSerializer($logo)->serialize()); }); } diff --git a/app/Http/Utils/FileUploadInfo.php b/app/Http/Utils/FileUploadInfo.php index 7c8a2ff89..969db6e21 100644 --- a/app/Http/Utils/FileUploadInfo.php +++ b/app/Http/Utils/FileUploadInfo.php @@ -47,19 +47,32 @@ final class FileUploadInfo */ private $filePath; + private $md5; + + private $mime_type; + + private $source_bucket; + /** * @param $file * @param $fileName * @param $fileExt * @param $filePath * @param $size + * @param $md5 + * @param $mime_type + * @param $source_bucket */ private function __construct( $file, $fileName, $fileExt, $filePath, - $size + $size, + $md5 = null, + $mime_type = null, + $source_bucket = null, + ) { $this->file = $file; @@ -67,6 +80,9 @@ private function __construct( $this->fileExt = $fileExt; $this->size = $size; $this->filePath = $filePath; + $this->md5 = $md5; + $this->mime_type = $mime_type; + $this->source_bucket = $source_bucket; } public static function getStorageDriver():string{ @@ -103,30 +119,59 @@ public static function build(LaravelRequest $request, array $payload):?FileUploa $fileExt = pathinfo($fileName, PATHINFO_EXTENSION); } - if(is_null($file) && isset($payload['filepath']) && !empty($payload['filepath'])){ - Log::debug(sprintf("FileUploadInfo::build build file is present on as %s storage (%s)", self::getStorageDriver(), $payload['filepath'])); + if(is_null($file)){ + return self::buildFromPayload($payload); + } + + $fileName = $fileName && !empty($fileName) ? FileNameSanitizer::sanitize($fileName) : $fileName; + + if(empty($filePath)) return null; + + return new self($file, $fileName, $fileExt, $filePath, $size); + } + + public static function buildFromPayload(array $payload):?FileUploadInfo{ + $filepath = null; + $filename = null; + $md5 = null; + $size = 0; + $mime_type = null; + $source_bucket = null; + $fileExt = null; + $file = null; + if(isset($payload['filepath']) && !empty($payload['filepath'])){ + $filepath = trim($payload['filepath']); + Log::debug(sprintf("FileUploadInfo::buildFromPayload file is present on as %s storage (%s)", self::getStorageDriver(), $filepath)); $disk = Storage::disk(self::getStorageDriver()); - if(!$disk->exists($payload['filepath'])) - throw new ValidationException(sprintf("file provide on filepath %s does not exists on %s storage.", self::getStorageDriver(), $payload['filepath'])); + if(!$disk->exists($payload['filepath'])) { + Log::warning(sprintf("FileUploadInfo::buildFromPayload file %s is not present at storage (%s)", self::getStorageDriver(), $filepath)); + throw new ValidationException(sprintf("file provide on filepath %s does not exists on %s storage.", self::getStorageDriver(), $filepath)); + } // get in bytes should be converted to KB - $size = $disk->size($payload['filepath']); - Log::debug(sprintf("FileUploadInfo::build build file %s storage (%s) size %s", self::getStorageDriver(), $payload['filepath'], $size)); - if($size == 0) + $size = isset($payload['size']) ? intval($payload['size']): $disk->size($filepath); + + Log::debug(sprintf("FileUploadInfo::buildFromPayload file %s storage (%s) size %s", self::getStorageDriver(), $payload['filepath'], $size)); + if($size == 0) { + Log::warning(sprintf("FileUploadInfo::buildFromPayload file %s size is zero", $filepath)); throw new ValidationException("File size is zero."); + } - $filePath = $payload['filepath']; - $fileName = pathinfo($payload['filepath'],PATHINFO_BASENAME); - $fileExt = pathinfo($fileName, PATHINFO_EXTENSION); - $file = self::isLocal() ? new UploadedFile($disk->path($payload['filepath']), $fileName): null; + + $filename = isset($payload['filename']) ? trim($payload['filename']): pathinfo($filepath, PATHINFO_BASENAME); + $md5 = isset($payload['md5']) ? trim($payload['md5']) : null; + $mime_type= isset($payload['mime_type']) ? trim($payload['mime_type']) : null; + $source_bucket = isset($payload['source_bucket']) ? trim($payload['source_bucket']) : null; + $fileExt = pathinfo($filename, PATHINFO_EXTENSION); + $file = self::isLocal() ? new UploadedFile($disk->path($payload['filepath']), $filename): null; } - $fileName = $fileName && !empty($fileName) ? FileNameSanitizer::sanitize($fileName) : $fileName; + $filename = $filename && !empty($filename) ? FileNameSanitizer::sanitize($filename) : $filename; - if(empty($filePath)) return null; + if(empty($filename) || empty($filepath)) return null; - return new self($file, $fileName, $fileExt, $filePath, $size); + return new self(null, $filename, $fileExt, $filepath, $size, $md5, $mime_type, $source_bucket); } /** @@ -175,9 +220,26 @@ public function delete(){ Log::debug(sprintf("FileUploadInfo::delete deleting file %s", $realPath)); } + public function getMd5(): ?string + { + return $this->md5; + } + + public function getMimeType(): ?string + { + return $this->mime_type; + } + + public function getSourceBucket(): ?string + { + return $this->source_bucket; + } + public function getFilePath(): ?string { return $this->filePath; } -} \ No newline at end of file + + +} diff --git a/app/Jobs/FileProcessingJob.php b/app/Jobs/FileProcessingJob.php new file mode 100644 index 000000000..883730f3f --- /dev/null +++ b/app/Jobs/FileProcessingJob.php @@ -0,0 +1,44 @@ +postProcessFileFromFileApi($this->fileInfoDTO); + } + + public function failed(\Throwable $exception) + { + Log::error(sprintf( "FileProcessingJob::failed %s", $exception->getMessage())); + } +} diff --git a/app/Models/Foundation/Main/Companies/Company.php b/app/Models/Foundation/Main/Companies/Company.php index 795e806c5..ecb4ce25b 100644 --- a/app/Models/Foundation/Main/Companies/Company.php +++ b/app/Models/Foundation/Main/Companies/Company.php @@ -25,6 +25,7 @@ #[ORM\Cache(usage: 'NONSTRICT_READ_WRITE', region: 'sponsors_region')] // Class Company class Company extends SilverstripeBaseModel { + public const LogoAllowedExtensions = ['png', 'jpg', 'jpeg', 'svg']; /** * @var string */ @@ -640,4 +641,4 @@ public function getUrlSegment(): ?string{ public function setUrlSegment(string $url_segment): void{ $this->url_segment = $url_segment; } -} \ No newline at end of file +} diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 17d2e8431..3c1c3b4e2 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -188,6 +188,25 @@ class AppServiceProvider extends ServiceProvider 'type_id' => 'required|integer', ]; + static $file_dto_fields = [ + 'filepath', + 'filename', + 'md5', + 'size', + 'mime_type', + 'source_bucket', + ]; + + static $file_dto_validation_rules = [ + 'filepath' => 'required|string', + 'filename' => 'required|string', + 'md5' => 'required|string', + 'size' => 'required|integer', + 'mime_type'=> 'sometimes|string', + 'source_bucket' => 'sometimes|string', + ]; + + /** * Bootstrap any application services. * @return void @@ -709,6 +728,14 @@ public function boot() return is_numeric($value) && intval($value) > - strlen(strval($value)) == 10; }); + Validator::replacer('file_dto', function ($message, $attribute, $rule, $parameters) { + return sprintf("%s should be a valid file dto (%s)", $attribute, implode(',', self::$file_dto_fields)); + }); + + Validator::extend('file_dto', function ($attribute, $value, $parameters, $validator) { + $validation = Validator::make($value, self::$file_dto_validation_rules); + return !$validation->fails(); + }); } /** diff --git a/app/Services/FilePostProcessorService.php b/app/Services/FilePostProcessorService.php new file mode 100644 index 000000000..4c7888271 --- /dev/null +++ b/app/Services/FilePostProcessorService.php @@ -0,0 +1,49 @@ +owner_entity_class, $file_info_dto->owner_member_name)); + $service = $this->locateService($file_info_dto->owner_entity_class); + if(is_null($service)){ + Log::warning(sprintf("FilePostProcessorService: no handler registered for entity class '%s'", $file_info_dto->owner_entity_class)); + throw new \InvalidArgumentException(sprintf("No file post-processor registered for entity class '%s'.", $file_info_dto->owner_entity_class)); + } + return $service->processFileForChildEntity($file_info_dto); + } +} diff --git a/app/Services/Model/FileInfoDTO.php b/app/Services/Model/FileInfoDTO.php new file mode 100644 index 000000000..9772f1954 --- /dev/null +++ b/app/Services/Model/FileInfoDTO.php @@ -0,0 +1,31 @@ +tx_service->transaction(function() use($payload){ + + $company = $this->tx_service->transaction(function() use($payload){ $company_name = trim($payload['name']); $former_company = $this->repository->getByName($company_name); if(!is_null($former_company)){ @@ -76,8 +88,55 @@ public function addCompany(array $payload): Company $this->repository->add($company); return $company; }); + + if(isset($payload['logo'])){ + $file_upload_info = FileUploadInfo::buildFromPayload($payload['logo']); + if(!is_null($file_upload_info)){ + if(!in_array($file_upload_info->getFileExt(),Company::LogoAllowedExtensions )) + throw new ValidationException(sprintf("Logo file does not has a valid extension (%s).", implode(',', Company::LogoAllowedExtensions))); + $job = new FileProcessingJob(new FileInfoDTO( + owner_entity_id : $company->getId(), + owner_entity_class: Company::class, + owner_member_name: "logo", + filepath: $file_upload_info->getFilePath(), + filename: $file_upload_info->getFileName(), + size: $file_upload_info->getSize(), + md5: $file_upload_info->getMd5(), + mime_type: $file_upload_info->getMimeType(), + source_bucket: $file_upload_info->getSourceBucket() + )); + JobDispatcher::withDbFallback( + job: $job, + ); + } + } + + if(isset($payload['big_logo'])){ + $file_upload_info = FileUploadInfo::buildFromPayload($payload['big_logo']); + if(!is_null($file_upload_info)){ + if(!in_array($file_upload_info->getFileExt(),Company::LogoAllowedExtensions )) + throw new ValidationException(sprintf("Big Logo file does not has a valid extension (%s).", implode(',', Company::LogoAllowedExtensions))); + $job = new FileProcessingJob(new FileInfoDTO( + owner_entity_id : $company->getId(), + owner_entity_class: Company::class, + owner_member_name: "big_logo", + filepath: $file_upload_info->getFilePath(), + filename: $file_upload_info->getFileName(), + size: $file_upload_info->getSize(), + md5: $file_upload_info->getMd5(), + mime_type: $file_upload_info->getMimeType(), + source_bucket: $file_upload_info->getSourceBucket() + )); + JobDispatcher::withDbFallback( + job: $job, + ); + } + } + + return $company; } + /** * @param int $company_id * @param array $payload @@ -97,6 +156,8 @@ public function updateCompany(int $company_id, array $payload): Company if(!is_null($former_company) && $company_id !== $former_company->getId()){ throw new ValidationException(sprintf("company %s already exists", $payload['name'])); } + + } return CompanyFactory::populate($company, $payload); @@ -128,16 +189,14 @@ public function addCompanyLogo(int $company_id, UploadedFile $file, $max_file_si { return $this->tx_service->transaction(function () use ($company_id, $file, $max_file_size) { - $allowed_extensions = ['png', 'jpg', 'jpeg', 'svg']; - $company = $this->repository->getById($company_id); if (is_null($company) || !$company instanceof Company) { throw new EntityNotFoundException('company not found!'); } - if (!in_array($file->extension(), $allowed_extensions)) { - throw new ValidationException("file does not has a valid extension ('png', 'jpg', 'jpeg', 'svg')."); + if (!in_array($file->extension(), Company::LogoAllowedExtensions)) { + throw new ValidationException(sprintf("file does not has a valid extension (%s).", implode(', ', Company::LogoAllowedExtensions))); } if ($file->getSize() > $max_file_size) { @@ -175,16 +234,14 @@ public function addCompanyBigLogo(int $company_id, UploadedFile $file, $max_file { return $this->tx_service->transaction(function () use ($company_id, $file, $max_file_size) { - $allowed_extensions = ['png', 'jpg', 'jpeg', 'svg']; - $company = $this->repository->getById($company_id); if (is_null($company) || !$company instanceof Company) { throw new EntityNotFoundException('company not found!'); } - if (!in_array($file->extension(), $allowed_extensions)) { - throw new ValidationException("file does not has a valid extension ('png', 'jpg', 'jpeg', 'svg')."); + if (!in_array($file->extension(), Company::LogoAllowedExtensions)) { + throw new ValidationException(sprintf("file does not has a valid extension (%s).", implode(', ', Company::LogoAllowedExtensions))); } if ($file->getSize() > $max_file_size) { @@ -214,4 +271,71 @@ public function deleteCompanyBigLogo(int $company_id): void }); } -} \ No newline at end of file + + /** + * @param FileInfoDTO $file_info_dto + * @return IEntity + * @throws EntityNotFoundException + * @throws ValidationException + */ + public function processFileForChildEntity(FileInfoDTO $file_info_dto): IEntity{ + Log::debug(sprintf("CompanyService::processFileForChildEntity file_info_dto %s", $file_info_dto )); + $logo = null; + switch($file_info_dto->owner_member_name){ + case 'big_logo': + $localPath = self::getFileFromRemoteStorageOnTempStorage( + $file_info_dto->filename, + $file_info_dto->filepath + ); + try { + if (!is_null($file_info_dto->md5)) { + $localHash = md5_file($localPath); + if ($localHash === false) + throw new ValidationException("File integrity check failed: unable to read local temp file."); + if ($localHash !== strtolower($file_info_dto->md5)) + throw new ValidationException("File integrity check failed: MD5 mismatch."); + } + $file = new UploadedFile( + path: $localPath, + originalName: $file_info_dto->filename, + mimeType: $file_info_dto->mime_type, + error: null, + test: true, + ); + $logo = $this->addCompanyBigLogo($file_info_dto->owner_entity_id, $file); + } finally { + self::cleanLocalAndRemoteFile($localPath, $file_info_dto->filepath); + } + return $logo; + case 'logo': + $localPath = self::getFileFromRemoteStorageOnTempStorage( + $file_info_dto->filename, + $file_info_dto->filepath + ); + + try { + if (!is_null($file_info_dto->md5)) { + $localHash = md5_file($localPath); + if ($localHash === false) + throw new ValidationException("File integrity check failed: unable to read local temp file."); + if ($localHash !== strtolower($file_info_dto->md5)) + throw new ValidationException("File integrity check failed: MD5 mismatch."); + } + $file = new UploadedFile( + path: $localPath, + originalName: $file_info_dto->filename, + mimeType: $file_info_dto->mime_type, + error: null, + test: true, + ); + $logo = $this->addCompanyLogo($file_info_dto->owner_entity_id, $file); + } finally { + self::cleanLocalAndRemoteFile($localPath, $file_info_dto->filepath); + } + return $logo; + default: + Log::warning(sprintf("CompanyService::processFileForChildEntity unknown member name '%s'", $file_info_dto->owner_member_name)); + throw new \InvalidArgumentException(sprintf("Unknown owner_member_name '%s' for entity class '%s'.", $file_info_dto->owner_member_name, $file_info_dto->owner_entity_class)); + } + } +} diff --git a/app/Services/ModelServicesProvider.php b/app/Services/ModelServicesProvider.php index 90eea8be5..fabcc4894 100644 --- a/app/Services/ModelServicesProvider.php +++ b/app/Services/ModelServicesProvider.php @@ -18,6 +18,7 @@ use App\Services\Apis\ExternalRegistrationFeeds\IExternalRegistrationFeedFactory; use App\Services\Apis\ExternalScheduleFeeds\ExternalScheduleFeedFactory; use App\Services\Apis\ExternalScheduleFeeds\IExternalScheduleFeedFactory; +use App\Services\FilePostProcessorService; use App\Services\FileSystem\IFileDownloadStrategy; use App\Services\FileSystem\IFileUploadStrategy; use App\Services\FileSystem\Swift\SwiftStorageFileDownloadStrategy; @@ -29,12 +30,12 @@ use App\Services\Model\IBadgeViewTypeService; use App\Services\Model\ICompanyService; use App\Services\Model\IElectionService; +use App\Services\Model\IFilePostProcessorService; use App\Services\Model\ILocationService; use App\Services\Model\IMemberService; use App\Services\Model\Imp\BadgeViewTypeService; use App\Services\Model\Imp\CompanyService; use App\Services\Model\Imp\ElectionService; -use App\Services\Model\Imp\Factories\RabbitPublisherFactory; use App\Services\Model\Imp\PaymentGatewayProfileService; use App\Services\Model\Imp\PresentationVideoMediaUploadProcessor; use App\Services\Model\Imp\ProcessScheduleEntityLifeCycleEventService; @@ -140,8 +141,6 @@ use App\Services\SummitRefundPolicyTypeService; use App\Services\SummitSponsorService; use App\Services\SummitSponsorshipService; -use App\Services\Utils\IPublisherService; -use App\Services\Utils\RabbitPublisherService; use Illuminate\Support\Facades\App; use Illuminate\Support\ServiceProvider; use services\model\ChatTeamService; @@ -529,6 +528,10 @@ public function register() ISummitRSVPService::class, SummitRSVPService::class ); + App::singleton( + IFilePostProcessorService::class, + FilePostProcessorService::class + ); } /** @@ -602,6 +605,7 @@ public function provides() ISponsorUserSyncService::class, ISummitRSVPService::class, ISummitRSVPInvitationService::class, + IFilePostProcessorService::class, ]; } -} \ No newline at end of file +} diff --git a/tests/Unit/Services/CompanyFileProcessingTest.php b/tests/Unit/Services/CompanyFileProcessingTest.php new file mode 100644 index 000000000..2dadb89f3 --- /dev/null +++ b/tests/Unit/Services/CompanyFileProcessingTest.php @@ -0,0 +1,240 @@ + InvalidArgumentException + * - processFileForChildEntity: MD5 mismatch -> "MD5 mismatch" (not "unable to read") + * - processFileForChildEntity: MD5 check skipped when md5 field is null + * - FilePostProcessorService: unknown entity class -> throws (not silent false) + * - FileInfoDTO: survives PHP queue serialization round-trip + * + * @package Tests\Unit\Services + */ +class CompanyFileProcessingTest extends TestCase +{ + private Container $app; + + protected function setUp(): void + { + parent::setUp(); + // Minimal facade application so Log::debug/warning calls resolve without + // a full Laravel app or database. clearResolvedInstances() first to drop + // any cached instance from a prior test that booted the full app. + Facade::clearResolvedInstances(); + $this->app = new Container(); + $this->app->singleton('log', fn() => new NullLogger()); + Container::setInstance($this->app); + Facade::setFacadeApplication($this->app); + } + + protected function tearDown(): void + { + Facade::setFacadeApplication(null); + Facade::clearResolvedInstances(); + Container::setInstance(null); + Mockery::close(); + parent::tearDown(); + } + + // ------------------------------------------------------------------------- + // Helpers + // ------------------------------------------------------------------------- + + private function makeCompanyService(): CompanyService + { + // CompanyService is final with a multi-arg constructor. Skip the + // constructor entirely - processFileForChildEntity only uses static + // trait methods and $this->addCompanyLogo/addCompanyBigLogo, neither + // of which is reached by the paths tested here. + $ref = new \ReflectionClass(CompanyService::class); + return $ref->newInstanceWithoutConstructor(); + } + + /** + * Register Storage and Config fakes so getFileFromRemoteStorageOnTempStorage + * and cleanLocalAndRemoteFile resolve without hitting real cloud storage. + * The disk mock serves an empty stream, producing a 0-byte local temp file. + */ + private function bindStorageFakes(string $remotePath): void + { + $emptyStream = fopen('php://temp', 'r+b'); + + $mockDisk = Mockery::mock(); + $mockDisk->shouldReceive('exists')->with($remotePath)->andReturn(true); + $mockDisk->shouldReceive('readStream')->with($remotePath)->andReturn($emptyStream); + $mockDisk->shouldReceive('size')->with($remotePath)->andReturn(0); // matches 0-byte local copy + $mockDisk->shouldReceive('delete')->with($remotePath)->andReturn(true); + + $mockFsFactory = Mockery::mock(); + $mockFsFactory->shouldReceive('disk')->andReturn($mockDisk); + + $mockConfig = Mockery::mock(); + $mockConfig->shouldReceive('get')->with('file_upload.storage_driver')->andReturn('s3'); + $mockConfig->shouldReceive('get')->withAnyArgs()->andReturn(null); + + $this->app->singleton('filesystem', fn() => $mockFsFactory); + $this->app->singleton('config', fn() => $mockConfig); + } + + private function makeLogoDto(string $memberName, ?string $md5 = null): FileInfoDTO + { + return new FileInfoDTO( + owner_entity_id: 1, + owner_entity_class: Company::class, + owner_member_name: $memberName, + filepath: 'companies/1/tmp/logo.png', + filename: 'logo.png', + size: 1024, + md5: $md5, + ); + } + + // ------------------------------------------------------------------------- + // processFileForChildEntity - routing / default case + // ------------------------------------------------------------------------- + + public function testProcessFileForChildEntityThrowsForUnknownMemberName(): void + { + $service = $this->makeCompanyService(); + + $dto = $this->makeLogoDto('avatar'); + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessageMatches('/avatar/'); + + $service->processFileForChildEntity($dto); + } + + // ------------------------------------------------------------------------- + // processFileForChildEntity - MD5 verification logic + // ------------------------------------------------------------------------- + + /** + * When the provided MD5 does not match the downloaded file's hash, + * the exception message must say "MD5 mismatch", NOT "unable to read local + * temp file". This guards against the regression where md5_file() returning + * false and an actual mismatch produced the same error message. + */ + public function testProcessFileForChildEntityThrowsMd5MismatchMessageOnHashDifference(): void + { + $remotePath = 'companies/1/tmp/logo.png'; + $this->bindStorageFakes($remotePath); + + $service = $this->makeCompanyService(); + + // md5 of a 0-byte file is d41d8cd98f00b204e9800998ecf8427e. + // Supplying a different hash triggers the mismatch branch. + $dto = $this->makeLogoDto('logo', 'ffffffffffffffffffffffffffffffff'); + + try { + $service->processFileForChildEntity($dto); + $this->fail('Expected ValidationException was not thrown.'); + } catch (ValidationException $e) { + $this->assertStringContainsString('MD5 mismatch', $e->getMessage()); + $this->assertStringNotContainsString('unable to read', $e->getMessage()); + } + } + + /** + * Same mismatch path for the big_logo member - ensures both switch cases + * share the same md5 check behaviour. + */ + public function testProcessFileForChildEntityMd5MismatchAlsoWorksForBigLogo(): void + { + $remotePath = 'companies/1/tmp/logo.png'; + $this->bindStorageFakes($remotePath); + + $service = $this->makeCompanyService(); + + $dto = $this->makeLogoDto('big_logo', 'ffffffffffffffffffffffffffffffff'); + + try { + $service->processFileForChildEntity($dto); + $this->fail('Expected ValidationException was not thrown.'); + } catch (ValidationException $e) { + $this->assertStringContainsString('MD5 mismatch', $e->getMessage()); + } + } + + // ------------------------------------------------------------------------- + // FilePostProcessorService - unknown entity class now throws, not silent false + // ------------------------------------------------------------------------- + + public function testPostProcessFileApiThrowsForUnknownEntityClass(): void + { + $service = new FilePostProcessorService(); + + $dto = new FileInfoDTO( + owner_entity_id: 1, + owner_entity_class: 'App\Models\SomeUnregisteredEntity', + owner_member_name: 'logo', + filepath: 'some/path', + filename: 'logo.png', + size: 1024, + ); + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessageMatches('/SomeUnregisteredEntity/'); + + $service->postProcessFileFromFileApi($dto); + } + + // ------------------------------------------------------------------------- + // FileInfoDTO - queue serialization round-trip + // ------------------------------------------------------------------------- + + public function testFileInfoDTOSurvivesQueueSerialization(): void + { + $original = new FileInfoDTO( + owner_entity_id: 42, + owner_entity_class: Company::class, + owner_member_name: 'logo', + filepath: 'companies/42/tmp/abc.png', + filename: 'logo.png', + size: 2048, + md5: 'abc123def456abc123def456abc123de', + mime_type: 'image/png', + source_bucket: 'my-bucket', + ); + + /** @var FileInfoDTO $copy */ + $copy = unserialize(serialize($original)); + + $this->assertSame(42, $copy->owner_entity_id); + $this->assertSame(Company::class, $copy->owner_entity_class); + $this->assertSame('logo', $copy->owner_member_name); + $this->assertSame('companies/42/tmp/abc.png', $copy->filepath); + $this->assertSame('logo.png', $copy->filename); + $this->assertSame(2048, $copy->size); + $this->assertSame('abc123def456abc123def456abc123de', $copy->md5); + $this->assertSame('image/png', $copy->mime_type); + $this->assertSame('my-bucket', $copy->source_bucket); + } +} diff --git a/tests/oauth2/OAuth2CompaniesApiTest.php b/tests/oauth2/OAuth2CompaniesApiTest.php index a36c2860e..e15efad12 100644 --- a/tests/oauth2/OAuth2CompaniesApiTest.php +++ b/tests/oauth2/OAuth2CompaniesApiTest.php @@ -12,6 +12,9 @@ * limitations under the License. **/ +use Illuminate\Http\UploadedFile; +use Illuminate\Support\Facades\Storage; + class OAuth2CompaniesApiTest extends ProtectedApiTestCase { @@ -71,8 +74,176 @@ public function testAddCompany(){ return $company; } - public function testAddCompanyBigLogo(){ + // ------------------------------------------------------------------------- + // Logo - dual flow (multipart + JSON payload) + // ------------------------------------------------------------------------- + + public function testAddCompanyLogoViaMultipartFile(): void + { + Storage::fake('public'); + Storage::fake('assets'); + + $company = $this->testAddCompany(); + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "multipart/form-data", + ]; + + $response = $this->action( + "POST", + "OAuth2CompaniesApiController@addCompanyLogo", + ['id' => $company->id], + [], + [], + ['file' => UploadedFile::fake()->image('logo.png')], + $headers + ); + + $this->assertResponseStatus(201); + $logo = json_decode($response->getContent()); + $this->assertNotNull($logo); + } + + public function testAddCompanyLogoViaJsonPayload(): void + { + Storage::fake('local'); + Storage::fake('public'); + Storage::fake('assets'); + + $company = $this->testAddCompany(); + + $fakeFile = UploadedFile::fake()->image('logo.png'); + $content = file_get_contents($fakeFile->getRealPath()); + $remotePath = 'companies/' . $company->id . '/tmp/logo.png'; + Storage::disk('local')->put($remotePath, $content); + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json", + ]; + + $response = $this->action( + "POST", + "OAuth2CompaniesApiController@addCompanyLogo", + ['id' => $company->id], + [], + [], + [], + $headers, + json_encode([ + 'filepath' => $remotePath, + 'filename' => 'logo.png', + 'md5' => md5($content), + 'size' => strlen($content), + 'mime_type' => 'image/png', + ]) + ); + + $this->assertResponseStatus(201); + $logo = json_decode($response->getContent()); + $this->assertNotNull($logo); + } + + public function testAddCompanyLogoJsonPayloadReturnsPreconditionFailedOnMissingFields(): void + { + $company = $this->testAddCompany(); + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json", + ]; + + // No file in request, and JSON body is missing all required fields + $response = $this->action( + "POST", + "OAuth2CompaniesApiController@addCompanyLogo", + ['id' => $company->id], + [], + [], + [], + $headers, + json_encode(['mime_type' => 'image/png']) + ); + + $this->assertResponseStatus(412); + } + + // ------------------------------------------------------------------------- + // Big logo - dual flow (multipart + JSON payload) + // ------------------------------------------------------------------------- + + public function testAddCompanyBigLogoViaMultipartFile(): void + { + Storage::fake('public'); + Storage::fake('assets'); + + $company = $this->testAddCompany(); + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "multipart/form-data", + ]; + + $response = $this->action( + "POST", + "OAuth2CompaniesApiController@addCompanyBigLogo", + ['id' => $company->id], + [], + [], + ['file' => UploadedFile::fake()->image('big_logo.png')], + $headers + ); + + $this->assertResponseStatus(201); + $logo = json_decode($response->getContent()); + $this->assertNotNull($logo); + } + + public function testAddCompanyBigLogoViaJsonPayload(): void + { + Storage::fake('local'); + Storage::fake('public'); + Storage::fake('assets'); + + $company = $this->testAddCompany(); + + $fakeFile = UploadedFile::fake()->image('big_logo.png'); + $content = file_get_contents($fakeFile->getRealPath()); + $remotePath = 'companies/' . $company->id . '/tmp/big_logo.png'; + Storage::disk('local')->put($remotePath, $content); + + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json", + ]; + + $response = $this->action( + "POST", + "OAuth2CompaniesApiController@addCompanyBigLogo", + ['id' => $company->id], + [], + [], + [], + $headers, + json_encode([ + 'filepath' => $remotePath, + 'filename' => 'big_logo.png', + 'md5' => md5($content), + 'size' => strlen($content), + 'mime_type' => 'image/png', + ]) + ); + + $this->assertResponseStatus(201); + $logo = json_decode($response->getContent()); + $this->assertNotNull($logo); + } + + // Kept for backward compatibility with any @depends on this method name + public function testAddCompanyBigLogo(): void + { + $this->testAddCompanyBigLogoViaMultipartFile(); } } \ No newline at end of file From f0f076f7be493137d249699a23aacbb4ef8fe8ab Mon Sep 17 00:00:00 2001 From: smarcet Date: Fri, 19 Jun 2026 17:41:56 -0300 Subject: [PATCH 2/5] docs(swagger): add FileDTO schema and logo/big_logo to company request 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 --- app/Swagger/CompaniesSchemas.php | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/app/Swagger/CompaniesSchemas.php b/app/Swagger/CompaniesSchemas.php index 76e020910..cd94c40a8 100644 --- a/app/Swagger/CompaniesSchemas.php +++ b/app/Swagger/CompaniesSchemas.php @@ -46,6 +46,24 @@ class PaginatedBaseCompaniesResponseSchema { } +#[OA\Schema( + schema: "FileDTO", + description: "File metadata payload produced by the File API after a successful upload", + required: ["filepath", "filename", "md5", "size"], + properties: [ + new OA\Property(property: "filepath", type: "string", description: "Remote storage path of the uploaded file", example: "companies/1/tmp/logo.png"), + new OA\Property(property: "filename", type: "string", description: "Original file name", example: "logo.png"), + new OA\Property(property: "md5", type: "string", description: "MD5 hash of the file for integrity verification", example: "d41d8cd98f00b204e9800998ecf8427e"), + new OA\Property(property: "size", type: "integer", description: "File size in bytes", example: 204800), + new OA\Property(property: "mime_type", type: "string", nullable: true, description: "MIME type of the file", example: "image/png"), + new OA\Property(property: "source_bucket", type: "string", nullable: true, description: "Source storage bucket", example: "my-uploads-bucket"), + ], + type: "object" +)] +class FileDTOSchema +{ +} + #[OA\Schema( schema: "CompanyCreateRequest", description: "Request to create a Company", @@ -69,6 +87,8 @@ class PaginatedBaseCompaniesResponseSchema new OA\Property(property: "overview", type: "string", nullable: true, example: "Company overview"), new OA\Property(property: "commitment", type: "string", nullable: true, example: "Commitment to open source"), new OA\Property(property: "commitment_author", type: "string", nullable: true, example: "John Doe, CEO"), + new OA\Property(property: "logo", nullable: true, ref: "#/components/schemas/FileDTO", description: "Company logo (File API payload)"), + new OA\Property(property: "big_logo", nullable: true, ref: "#/components/schemas/FileDTO", description: "Company big logo (File API payload)"), ], type: "object" )] @@ -98,6 +118,8 @@ class CompanyCreateRequestSchema new OA\Property(property: "overview", type: "string", nullable: true, example: "Company overview"), new OA\Property(property: "commitment", type: "string", nullable: true, example: "Commitment to open source"), new OA\Property(property: "commitment_author", type: "string", nullable: true, example: "John Doe, CEO"), + new OA\Property(property: "logo", nullable: true, ref: "#/components/schemas/FileDTO", description: "Company logo (File API payload)"), + new OA\Property(property: "big_logo", nullable: true, ref: "#/components/schemas/FileDTO", description: "Company big logo (File API payload)"), ], type: "object" )] From 7db85e52ca335f60b71159d6746b30fef478ba36 Mon Sep 17 00:00:00 2001 From: smarcet Date: Fri, 19 Jun 2026 17:43:04 -0300 Subject: [PATCH 3/5] refactor(swagger): move FileDTO schema to its own file --- app/Swagger/CompaniesSchemas.php | 17 ----------------- app/Swagger/FileDTOSchema.php | 23 +++++++++++++++++++++++ 2 files changed, 23 insertions(+), 17 deletions(-) create mode 100644 app/Swagger/FileDTOSchema.php diff --git a/app/Swagger/CompaniesSchemas.php b/app/Swagger/CompaniesSchemas.php index cd94c40a8..9ff05fcc4 100644 --- a/app/Swagger/CompaniesSchemas.php +++ b/app/Swagger/CompaniesSchemas.php @@ -46,23 +46,6 @@ class PaginatedBaseCompaniesResponseSchema { } -#[OA\Schema( - schema: "FileDTO", - description: "File metadata payload produced by the File API after a successful upload", - required: ["filepath", "filename", "md5", "size"], - properties: [ - new OA\Property(property: "filepath", type: "string", description: "Remote storage path of the uploaded file", example: "companies/1/tmp/logo.png"), - new OA\Property(property: "filename", type: "string", description: "Original file name", example: "logo.png"), - new OA\Property(property: "md5", type: "string", description: "MD5 hash of the file for integrity verification", example: "d41d8cd98f00b204e9800998ecf8427e"), - new OA\Property(property: "size", type: "integer", description: "File size in bytes", example: 204800), - new OA\Property(property: "mime_type", type: "string", nullable: true, description: "MIME type of the file", example: "image/png"), - new OA\Property(property: "source_bucket", type: "string", nullable: true, description: "Source storage bucket", example: "my-uploads-bucket"), - ], - type: "object" -)] -class FileDTOSchema -{ -} #[OA\Schema( schema: "CompanyCreateRequest", diff --git a/app/Swagger/FileDTOSchema.php b/app/Swagger/FileDTOSchema.php new file mode 100644 index 000000000..bb488d321 --- /dev/null +++ b/app/Swagger/FileDTOSchema.php @@ -0,0 +1,23 @@ + Date: Fri, 19 Jun 2026 18:01:43 -0300 Subject: [PATCH 4/5] fix: address CodeRabbit review findings - 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 --- Libs/Utils/FileUtils.php | 13 +++++++++++++ app/Http/Utils/FileUploadInfo.php | 10 +++++----- app/Jobs/FileProcessingJob.php | 5 ++++- app/Services/Model/FileInfoDTO.php | 2 +- app/Services/Model/Imp/CompanyService.php | 19 ++++++++++++++++--- tests/oauth2/OAuth2CompaniesApiTest.php | 3 +++ 6 files changed, 42 insertions(+), 10 deletions(-) diff --git a/Libs/Utils/FileUtils.php b/Libs/Utils/FileUtils.php index cc614bc27..9b4b1f243 100644 --- a/Libs/Utils/FileUtils.php +++ b/Libs/Utils/FileUtils.php @@ -137,4 +137,17 @@ public static function cleanLocalAndRemoteFile(string $localPath, string $remote Log::debug(sprintf("cleanLocalFile deleting local file %s", $localPath)); unlink($localPath); } + + /** + * Deletes only the local temp file. Use in finally blocks where the remote + * file must be preserved on failure so queue job retries can re-download it. + * @param string $localPath + * @return void + */ + public static function cleanLocalFile(string $localPath): void { + if (file_exists($localPath)) { + Log::debug(sprintf("FileUtils::cleanLocalFile deleting local temp file %s", $localPath)); + @unlink($localPath); + } + } } \ No newline at end of file diff --git a/app/Http/Utils/FileUploadInfo.php b/app/Http/Utils/FileUploadInfo.php index 969db6e21..80d98d4ec 100644 --- a/app/Http/Utils/FileUploadInfo.php +++ b/app/Http/Utils/FileUploadInfo.php @@ -144,15 +144,15 @@ public static function buildFromPayload(array $payload):?FileUploadInfo{ Log::debug(sprintf("FileUploadInfo::buildFromPayload file is present on as %s storage (%s)", self::getStorageDriver(), $filepath)); $disk = Storage::disk(self::getStorageDriver()); - if(!$disk->exists($payload['filepath'])) { + if(!$disk->exists($filepath)) { Log::warning(sprintf("FileUploadInfo::buildFromPayload file %s is not present at storage (%s)", self::getStorageDriver(), $filepath)); throw new ValidationException(sprintf("file provide on filepath %s does not exists on %s storage.", self::getStorageDriver(), $filepath)); } - // get in bytes should be converted to KB - $size = isset($payload['size']) ? intval($payload['size']): $disk->size($filepath); + // Always retrieve the actual size from storage; never trust client-provided value. + $size = $disk->size($filepath); - Log::debug(sprintf("FileUploadInfo::buildFromPayload file %s storage (%s) size %s", self::getStorageDriver(), $payload['filepath'], $size)); + Log::debug(sprintf("FileUploadInfo::buildFromPayload file %s storage (%s) size %s", self::getStorageDriver(), $filepath, $size)); if($size == 0) { Log::warning(sprintf("FileUploadInfo::buildFromPayload file %s size is zero", $filepath)); throw new ValidationException("File size is zero."); @@ -164,7 +164,7 @@ public static function buildFromPayload(array $payload):?FileUploadInfo{ $mime_type= isset($payload['mime_type']) ? trim($payload['mime_type']) : null; $source_bucket = isset($payload['source_bucket']) ? trim($payload['source_bucket']) : null; $fileExt = pathinfo($filename, PATHINFO_EXTENSION); - $file = self::isLocal() ? new UploadedFile($disk->path($payload['filepath']), $filename): null; + $file = self::isLocal() ? new UploadedFile($disk->path($filepath), $filename): null; } $filename = $filename && !empty($filename) ? FileNameSanitizer::sanitize($filename) : $filename; diff --git a/app/Jobs/FileProcessingJob.php b/app/Jobs/FileProcessingJob.php index 883730f3f..a1de086de 100644 --- a/app/Jobs/FileProcessingJob.php +++ b/app/Jobs/FileProcessingJob.php @@ -34,7 +34,10 @@ public function __construct( ) {} public function handle(IFilePostProcessorService $service){ - $service->postProcessFileFromFileApi($this->fileInfoDTO); + $result = $service->postProcessFileFromFileApi($this->fileInfoDTO); + if (!$result) { + throw new \RuntimeException(sprintf("FileProcessingJob: postProcessFileFromFileApi returned false for %s", $this->fileInfoDTO)); + } } public function failed(\Throwable $exception) diff --git a/app/Services/Model/FileInfoDTO.php b/app/Services/Model/FileInfoDTO.php index 9772f1954..2bb56cd66 100644 --- a/app/Services/Model/FileInfoDTO.php +++ b/app/Services/Model/FileInfoDTO.php @@ -26,6 +26,6 @@ public function __construct( ) {} public function __toString(): string { - return json_encode(get_object_vars($this), JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES); + return json_encode(get_object_vars($this), JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES) ?: '{}'; } } diff --git a/app/Services/Model/Imp/CompanyService.php b/app/Services/Model/Imp/CompanyService.php index 57e85f024..dd402c5cd 100644 --- a/app/Services/Model/Imp/CompanyService.php +++ b/app/Services/Model/Imp/CompanyService.php @@ -287,6 +287,7 @@ public function processFileForChildEntity(FileInfoDTO $file_info_dto): IEntity{ $file_info_dto->filename, $file_info_dto->filepath ); + $succeeded = false; try { if (!is_null($file_info_dto->md5)) { $localHash = md5_file($localPath); @@ -303,8 +304,14 @@ public function processFileForChildEntity(FileInfoDTO $file_info_dto): IEntity{ test: true, ); $logo = $this->addCompanyBigLogo($file_info_dto->owner_entity_id, $file); + $succeeded = true; } finally { - self::cleanLocalAndRemoteFile($localPath, $file_info_dto->filepath); + // Remote file preserved on failure so queue retries can re-download it. + if ($succeeded) { + self::cleanLocalAndRemoteFile($localPath, $file_info_dto->filepath); + } else { + self::cleanLocalFile($localPath); + } } return $logo; case 'logo': @@ -312,7 +319,7 @@ public function processFileForChildEntity(FileInfoDTO $file_info_dto): IEntity{ $file_info_dto->filename, $file_info_dto->filepath ); - + $succeeded = false; try { if (!is_null($file_info_dto->md5)) { $localHash = md5_file($localPath); @@ -329,8 +336,14 @@ public function processFileForChildEntity(FileInfoDTO $file_info_dto): IEntity{ test: true, ); $logo = $this->addCompanyLogo($file_info_dto->owner_entity_id, $file); + $succeeded = true; } finally { - self::cleanLocalAndRemoteFile($localPath, $file_info_dto->filepath); + // Remote file preserved on failure so queue retries can re-download it. + if ($succeeded) { + self::cleanLocalAndRemoteFile($localPath, $file_info_dto->filepath); + } else { + self::cleanLocalFile($localPath); + } } return $logo; default: diff --git a/tests/oauth2/OAuth2CompaniesApiTest.php b/tests/oauth2/OAuth2CompaniesApiTest.php index e15efad12..ace1f4241 100644 --- a/tests/oauth2/OAuth2CompaniesApiTest.php +++ b/tests/oauth2/OAuth2CompaniesApiTest.php @@ -13,6 +13,7 @@ **/ use Illuminate\Http\UploadedFile; +use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Storage; class OAuth2CompaniesApiTest extends ProtectedApiTestCase @@ -107,6 +108,7 @@ public function testAddCompanyLogoViaMultipartFile(): void public function testAddCompanyLogoViaJsonPayload(): void { + Config::set('file_upload.storage_driver', 'local'); Storage::fake('local'); Storage::fake('public'); Storage::fake('assets'); @@ -202,6 +204,7 @@ public function testAddCompanyBigLogoViaMultipartFile(): void public function testAddCompanyBigLogoViaJsonPayload(): void { + Config::set('file_upload.storage_driver', 'local'); Storage::fake('local'); Storage::fake('public'); Storage::fake('assets'); From 4e70016fdde38c0fffa064a9a4451a115b24f61e Mon Sep 17 00:00:00 2001 From: smarcet Date: Fri, 19 Jun 2026 18:03:11 -0300 Subject: [PATCH 5/5] fix: adversarial review findings - 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 --- Libs/Utils/FileUtils.php | 2 +- app/Jobs/FileProcessingJob.php | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/Libs/Utils/FileUtils.php b/Libs/Utils/FileUtils.php index 9b4b1f243..322ebedd4 100644 --- a/Libs/Utils/FileUtils.php +++ b/Libs/Utils/FileUtils.php @@ -118,7 +118,7 @@ public static function getFileFromRemoteStorageOnTempStorage(string $file_name, ); } } catch (\Throwable $e) { - throw new ValidationException($e->getMessage()); + throw new ValidationException($e->getMessage(), 0, $e); } return $localPath; diff --git a/app/Jobs/FileProcessingJob.php b/app/Jobs/FileProcessingJob.php index a1de086de..45a955587 100644 --- a/app/Jobs/FileProcessingJob.php +++ b/app/Jobs/FileProcessingJob.php @@ -28,6 +28,10 @@ class FileProcessingJob implements ShouldQueue public $timeout = 600; + public function backoff(): array { + return [30, 60]; + } + public function __construct( public readonly FileInfoDTO $fileInfoDTO,