diff --git a/src/Export/Service/DownloadService.php b/src/Export/Service/DownloadService.php index a38d2206b..d004a1d52 100644 --- a/src/Export/Service/DownloadService.php +++ b/src/Export/Service/DownloadService.php @@ -25,6 +25,7 @@ use Pimcore\Bundle\StudioBackendBundle\Util\Constant\HttpResponseHeaders; use Pimcore\Bundle\StudioBackendBundle\Util\Trait\StreamedResponseTrait; use Pimcore\Bundle\StudioBackendBundle\Util\Trait\TempFilePathTrait; +use Psr\Log\LoggerInterface; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpFoundation\StreamedResponse; use function sprintf; @@ -39,7 +40,8 @@ public function __construct( private ExecutionEngineServiceInterface $executionEngineService, - private StorageServiceInterface $storageService + private LoggerInterface $logger, + private StorageServiceInterface $storageService, ) { } @@ -59,27 +61,27 @@ public function downloadResourceByJobRunId( $filePath = $folderName . '/' . $fileName; $storage = $this->validateStorage($filePath, $jobRunId); - $streamedResponse = $this->getFileStreamedResponse( - $filePath, - $mimeType, - $downloadName, - $storage + return $this->getFileStreamedResponse( + path: $filePath, + mimeType: $mimeType, + filename: $downloadName, + storage: $storage, + onStreamComplete: function () use ($storage, $filePath, $folderName, $jobRunId): void { + try { + $storage->delete($filePath); + $this->storageService->cleanUpFolder($folderName); + $this->executionEngineService->hideJobRun($jobRunId); + } catch (FilesystemException $e) { + $this->logger->error( + 'Failed to clean up temporary folder {folder}', + [ + 'folder' => $folderName, + 'exception' => $e, + ] + ); + } + }, ); - - try { - $storage->delete($filePath); - $this->storageService->cleanUpFolder($folderName); - $this->executionEngineService->hideJobRun($jobRunId); - } catch (FilesystemException) { - throw new EnvironmentException( - sprintf( - 'Failed to clean up temporary folder %s', - $folderName - ) - ); - } - - return $streamedResponse; } /** diff --git a/src/Util/Trait/StreamedResponseTrait.php b/src/Util/Trait/StreamedResponseTrait.php index a90f55b8d..69ad7b2a3 100644 --- a/src/Util/Trait/StreamedResponseTrait.php +++ b/src/Util/Trait/StreamedResponseTrait.php @@ -23,6 +23,7 @@ use Pimcore\Model\Asset\Thumbnail\ImageThumbnailInterface; use Pimcore\Model\Asset\Video; use Pimcore\Model\Asset\Video\ImageThumbnailInterface as VideoImageThumbnailInterface; +use Closure; use Symfony\Component\HttpFoundation\StreamedResponse; use function is_resource; use function sprintf; @@ -42,11 +43,12 @@ protected function getStreamedResponse( ?int $fileSize = null, ): StreamedResponse { $stream = $element->getStream(); + $asset = $this->getAsset($element); if (!is_resource($stream)) { throw new ElementStreamResourceNotFoundException( - $element->getId(), - $element->getType() + $asset->getId(), + $asset->getType() ); } @@ -62,7 +64,7 @@ function () use ($stream) { $this->getResponseHeaders( mimeType: $element->getMimeType(), fileSize: $fileSize, - filename: $element->getFilename(), + filename: $asset->getFilename(), contentDisposition: $contentDisposition, additionalHeaders: $additionalHeaders ) @@ -106,13 +108,17 @@ protected function getFileStreamedResponse( string $filename, FilesystemOperator $storage, string $contentDisposition = HttpResponseHeaders::ATTACHMENT_TYPE->value, + ?Closure $onStreamComplete = null, ): StreamedResponse { try { $stream = $storage->readStream($path); return new StreamedResponse( - function () use ($stream) { + function () use ($stream, $onStreamComplete) { fpassthru($stream); + if ($onStreamComplete !== null) { + $onStreamComplete(); + } }, HttpResponseCodes::SUCCESS->value, $this->getResponseHeaders( @@ -135,6 +141,15 @@ function () use ($stream) { } + private function getAsset(Asset|VideoImageThumbnailInterface|ImageThumbnailInterface $element): Asset + { + if ($element instanceof Asset) { + return $element; + } + + return $element->getAsset(); + } + private function getResponseHeaders( string $mimeType, int $fileSize, diff --git a/tests/Unit/Export/Service/DownloadServiceTest.php b/tests/Unit/Export/Service/DownloadServiceTest.php new file mode 100644 index 000000000..ed2df5775 --- /dev/null +++ b/tests/Unit/Export/Service/DownloadServiceTest.php @@ -0,0 +1,419 @@ +makeEmpty(FilesystemOperator::class, [ + 'readStream' => $stream, + 'fileSize' => 12, + ]); + + $service = $this->createService( + storageService: $this->makeEmpty(StorageServiceInterface::class, [ + 'getTempStorage' => $storage, + 'tempFileExists' => true, + ]), + ); + + $response = $service->downloadResourceByJobRunId( + jobRunId: 1, + tempFileName: 'export_{id}.zip', + tempFolderName: 'export_{id}', + mimeType: 'application/zip', + downloadName: 'download.zip', + ); + + $this->assertInstanceOf(StreamedResponse::class, $response); + } + + + /** + * @throws Exception + */ + public function testCleanupRunsAfterStreamingNotBefore(): void + { + $stream = fopen('php://memory', 'rb+'); + fwrite($stream, 'test content'); + rewind($stream); + + $callOrder = []; + + $storage = $this->makeEmpty(FilesystemOperator::class, [ + 'readStream' => $stream, + 'fileSize' => 12, + 'delete' => function () use (&$callOrder) { + $callOrder[] = 'delete'; + }, + ]); + + $storageService = $this->makeEmpty(StorageServiceInterface::class, [ + 'getTempStorage' => $storage, + 'tempFileExists' => true, + 'cleanUpFolder' => function () use (&$callOrder) { + $callOrder[] = 'cleanUpFolder'; + }, + ]); + + $executionEngineService = $this->makeEmpty(ExecutionEngineServiceInterface::class, [ + 'validateJobRun' => null, + 'hideJobRun' => function () use (&$callOrder) { + $callOrder[] = 'hideJobRun'; + }, + ]); + + $service = $this->createService( + executionEngineService: $executionEngineService, + storageService: $storageService, + ); + + $response = $service->downloadResourceByJobRunId( + jobRunId: 1, + tempFileName: 'export_{id}.zip', + tempFolderName: 'export_{id}', + mimeType: 'application/zip', + downloadName: 'download.zip', + ); + + // Before streaming, no cleanup should have happened + $this->assertEmpty($callOrder, 'Cleanup must not run before the response is streamed'); + + // Now stream the response (simulates what Symfony does on send) + ob_start(); + $response->sendContent(); + ob_end_clean(); + + // After streaming, cleanup should have run in order + $this->assertSame( + ['delete', 'cleanUpFolder', 'hideJobRun'], + $callOrder, + 'Cleanup must run after streaming completes, in the correct order' + ); + } + + /** + * @throws Exception + */ + public function testCleanupFailureIsLoggedNotThrown(): void + { + $stream = fopen('php://memory', 'rb+'); + fwrite($stream, 'test content'); + rewind($stream); + + $storage = $this->makeEmpty(FilesystemOperator::class, [ + 'readStream' => $stream, + 'fileSize' => 12, + 'delete' => function () { + throw UnableToDeleteFile::atLocation('test', 'Simulated failure'); + }, + ]); + + $logger = $this->makeEmpty(LoggerInterface::class, [ + 'error' => Expected::once(), + ]); + + $service = $this->createService( + storageService: $this->makeEmpty(StorageServiceInterface::class, [ + 'getTempStorage' => $storage, + 'tempFileExists' => true, + ]), + logger: $logger, + ); + + $response = $service->downloadResourceByJobRunId( + jobRunId: 1, + tempFileName: 'export_{id}.zip', + tempFolderName: 'export_{id}', + mimeType: 'application/zip', + downloadName: 'download.zip', + ); + + // Streaming should not throw - the error is logged instead + ob_start(); + $response->sendContent(); + ob_end_clean(); + } + + /** + * @throws Exception + */ + public function testThrowsEnvironmentExceptionWhenTempFileNotFound(): void + { + $this->expectException(EnvironmentException::class); + + $service = $this->createService( + storageService: $this->makeEmpty(StorageServiceInterface::class, [ + 'getTempStorage' => $this->makeEmpty(FilesystemOperator::class), + 'tempFileExists' => false, + ]), + ); + + $service->downloadResourceByJobRunId( + jobRunId: 1, + tempFileName: 'export_{id}.zip', + tempFolderName: 'export_{id}', + mimeType: 'application/zip', + downloadName: 'download.zip', + ); + } + + /** + * @throws Exception + */ + public function testDownloadResourceSubstitutesJobRunIdInFilePaths(): void + { + $actualPath = null; + + $storage = $this->makeEmpty(FilesystemOperator::class, [ + 'readStream' => function (string $path) use (&$actualPath) { + $actualPath = $path; + + $stream = fopen('php://memory', 'rb+'); + fwrite($stream, 'x'); + rewind($stream); + + return $stream; + }, + 'fileSize' => 1, + ]); + + $service = $this->createService( + storageService: $this->makeEmpty(StorageServiceInterface::class, [ + 'getTempStorage' => $storage, + 'tempFileExists' => true, + ]), + ); + + $service->downloadResourceByJobRunId( + jobRunId: 42, + tempFileName: 'export_{id}.zip', + tempFolderName: 'folder_{id}', + mimeType: 'application/zip', + downloadName: 'download.zip', + ); + + $this->assertSame( + 'folder_42/export_42.zip', + $actualPath, + 'Job run ID must be substituted into both folder and file name placeholders' + ); + } + + /** + * @throws Exception + */ + public function testDownloadResourceSetsCorrectResponseHeaders(): void + { + $stream = fopen('php://memory', 'rb+'); + fwrite($stream, 'file-content'); + rewind($stream); + + $storage = $this->makeEmpty(FilesystemOperator::class, [ + 'readStream' => $stream, + 'fileSize' => 12, + ]); + + $service = $this->createService( + storageService: $this->makeEmpty(StorageServiceInterface::class, [ + 'getTempStorage' => $storage, + 'tempFileExists' => true, + ]), + ); + + $response = $service->downloadResourceByJobRunId( + jobRunId: 1, + tempFileName: 'export_{id}.zip', + tempFolderName: 'export_{id}', + mimeType: 'application/zip', + downloadName: 'my-export.zip', + ); + + $this->assertSame(200, $response->getStatusCode()); + $this->assertSame('application/zip', $response->headers->get('Content-Type')); + $this->assertSame('12', $response->headers->get('Content-Length')); + $this->assertSame( + 'attachment; filename="my-export.zip"', + $response->headers->get('Content-Disposition'), + 'Content-Disposition must use the downloadName parameter, not the internal temp file name' + ); + } + + /** + * @throws Exception + */ + public function testThrowsEnvironmentExceptionWithJobRunIdInMessage(): void + { + $service = $this->createService( + storageService: $this->makeEmpty(StorageServiceInterface::class, [ + 'getTempStorage' => $this->makeEmpty(FilesystemOperator::class), + 'tempFileExists' => false, + ]), + ); + + try { + $service->downloadResourceByJobRunId( + jobRunId: 99, + tempFileName: 'export_{id}.zip', + tempFolderName: 'export_{id}', + mimeType: 'application/zip', + downloadName: 'download.zip', + ); + $this->fail('Expected EnvironmentException was not thrown'); + } catch (EnvironmentException $e) { + $this->assertStringContainsString('99', $e->getMessage()); + } + } + + /** + * @throws Exception + */ + public function testDownloadJsonSetsBodyAndHeaders(): void + { + $json = '{"users":[1,2,3]}'; + $service = $this->createService(); + + $response = $service->downloadJSON($json, 'export-users.json'); + + $this->assertSame($json, $response->getContent()); + $this->assertSame( + MimeTypes::JSON->value, + $response->headers->get(HttpResponseHeaders::HEADER_CONTENT_TYPE->value) + ); + $this->assertSame( + 'attachment; filename="export-users.json"', + $response->headers->get(HttpResponseHeaders::HEADER_CONTENT_DISPOSITION->value) + ); + } + + /** + * @throws Exception + */ + public function testDownloadJsonPreservesSpecialCharactersInFilename(): void + { + $service = $this->createService(); + + $response = $service->downloadJSON('[]', 'file with spaces (1).json'); + + $this->assertSame( + 'attachment; filename="file with spaces (1).json"', + $response->headers->get('Content-Disposition') + ); + } + + /** + * @throws Exception + */ + public function testCleanupDataByJobRunIdException(): void + { + $storageService = $this->makeEmpty(StorageServiceInterface::class, [ + 'getTempStorage' => $this->makeEmpty(FilesystemOperator::class), + 'tempFileExists' => true, + 'cleanUpFolder' => function () { + throw UnableToDeleteFile::atLocation('some/path', 'disk full'); + }, + ]); + + $service = $this->createService(storageService: $storageService); + + try { + $service->cleanupDataByJobRunId( + jobRunId: 7, + folderName: 'folder_{id}', + fileName: 'file_{id}.zip', + ); + $this->fail('Expected EnvironmentException was not thrown'); + } catch (EnvironmentException $e) { + $this->assertStringContainsString('7', $e->getMessage()); + $this->assertStringContainsString('Failed to delete file', $e->getMessage()); + } + } + + /** + * @throws Exception + */ + public function testCleanupDataByJobRunId(): void + { + $capturedRemoveContents = null; + + $storageService = $this->makeEmpty(StorageServiceInterface::class, [ + 'getTempStorage' => $this->makeEmpty(FilesystemOperator::class), + 'tempFileExists' => true, + 'cleanUpFolder' => function (string $folder, bool $removeContents) use (&$capturedRemoveContents) { + $capturedRemoveContents = $removeContents; + }, + ]); + + $executionEngineService = $this->makeEmpty(ExecutionEngineServiceInterface::class, [ + 'hideJobRun' => Expected::once(), + ]); + + $service = $this->createService( + executionEngineService: $executionEngineService, + storageService: $storageService, + ); + + $service->cleanupDataByJobRunId( + jobRunId: 1, + folderName: 'folder_{id}', + fileName: 'file_{id}.zip', + ); + + $this->assertTrue( + $capturedRemoveContents, + 'cleanupDataByJobRunId must call cleanUpFolder with removeContents=true' + ); + } + + /** + * @throws Exception + */ + private function createService( + ?ExecutionEngineServiceInterface $executionEngineService = null, + ?StorageServiceInterface $storageService = null, + ?LoggerInterface $logger = null, + ): DownloadService { + return new DownloadService( + $executionEngineService ?? $this->makeEmpty(ExecutionEngineServiceInterface::class), + $logger ?? $this->makeEmpty(LoggerInterface::class), + $storageService ?? $this->makeEmpty(StorageServiceInterface::class), + ); + } +} diff --git a/tests/Unit/Util/Trait/StreamedResponseTraitTest.php b/tests/Unit/Util/Trait/StreamedResponseTraitTest.php new file mode 100644 index 000000000..0974250bb --- /dev/null +++ b/tests/Unit/Util/Trait/StreamedResponseTraitTest.php @@ -0,0 +1,230 @@ +createStorageWithContent('hello'); + + $response = $this->createTraitHelper()->callGetFileStreamedResponse( + path: 'test/file.zip', + mimeType: 'application/zip', + filename: 'file.zip', + storage: $storage, + onStreamComplete: function () use (&$callbackCalled) { + $callbackCalled = true; + }, + ); + + $this->assertFalse($callbackCalled, 'Callback must not be called before streaming'); + + ob_start(); + $response->sendContent(); + ob_end_clean(); + + $this->assertTrue($callbackCalled, 'Callback must be called after streaming completes'); + } + + /** + * @throws Exception + */ + public function testGetFileStreamedResponseWorksWithoutCallback(): void + { + $storage = $this->createStorageWithContent('data'); + + $response = $this->createTraitHelper()->callGetFileStreamedResponse( + path: 'test/file.csv', + mimeType: 'text/csv', + filename: 'file.csv', + storage: $storage, + ); + + ob_start(); + $response->sendContent(); + $output = ob_get_clean(); + + $this->assertSame('data', $output); + } + + /** + * @throws Exception + */ + public function testGetFileStreamedResponseError(): void + { + $this->expectException(StreamResourceNotFoundException::class); + + $storage = $this->makeEmpty(FilesystemOperator::class, [ + 'readStream' => function () { + throw UnableToReadFile::fromLocation('missing', 'disk error'); + }, + ]); + + $this->createTraitHelper()->callGetFileStreamedResponse( + path: 'missing/file.zip', + mimeType: 'application/zip', + filename: 'file.zip', + storage: $storage, + ); + } + + /** + * @throws Exception + */ + public function testResponseHeadersAreCorrectlyConstructed(): void + { + $content = 'hello world!'; + $storage = $this->createStorageWithContent($content); + + $response = $this->createTraitHelper()->callGetFileStreamedResponse( + path: 'data/export.csv', + mimeType: 'text/csv', + filename: 'quarterly-report.csv', + storage: $storage, + ); + + $this->assertSame('text/csv', $response->headers->get('Content-Type')); + $this->assertSame( + (string) strlen($content), + $response->headers->get('Content-Length') + ); + $this->assertSame( + 'attachment; filename="quarterly-report.csv"', + $response->headers->get('Content-Disposition'), + 'Default content disposition must be "attachment" with quoted filename' + ); + } + + /** + * @throws Exception + */ + public function testDefaultContentDispositionIsAttachment(): void + { + $storage = $this->createStorageWithContent('x'); + + $response = $this->createTraitHelper()->callGetFileStreamedResponse( + path: 'test/file.pdf', + mimeType: 'application/pdf', + filename: 'document.pdf', + storage: $storage, + ); + + $disposition = $response->headers->get('Content-Disposition'); + $this->assertStringStartsWith( + HttpResponseHeaders::ATTACHMENT_TYPE->value . ';', + $disposition + ); + } + + /** + * @throws Exception + */ + public function testCustomContentDispositionIsUsed(): void + { + $storage = $this->createStorageWithContent('x'); + + $response = $this->createTraitHelper()->callGetFileStreamedResponse( + path: 'test/image.jpg', + mimeType: 'image/jpeg', + filename: 'photo.jpg', + storage: $storage, + contentDisposition: HttpResponseHeaders::INLINE_TYPE->value, + ); + + $this->assertSame( + 'inline; filename="photo.jpg"', + $response->headers->get('Content-Disposition') + ); + } + + /** + * @throws Exception + */ + public function testStreamedContentMatchesSourceData(): void + { + $binaryContent = str_repeat("\x00\xFF\xAB", 100); + $storage = $this->createStorageWithContent($binaryContent); + + $response = $this->createTraitHelper()->callGetFileStreamedResponse( + path: 'test/binary.dat', + mimeType: 'application/octet-stream', + filename: 'binary.dat', + storage: $storage, + ); + + ob_start(); + $response->sendContent(); + $output = ob_get_clean(); + + $this->assertSame($binaryContent, $output, 'Streamed output must match source data byte-for-byte'); + } + + /** + * @throws Exception + */ + private function createStorageWithContent(string $content): FilesystemOperator + { + $stream = fopen('php://memory', 'rb+'); + fwrite($stream, $content); + rewind($stream); + + return $this->makeEmpty(FilesystemOperator::class, [ + 'readStream' => $stream, + 'fileSize' => strlen($content), + ]); + } + + private function createTraitHelper(): object + { + return new class { + use StreamedResponseTrait; + + public function callGetFileStreamedResponse( + string $path, + string $mimeType, + string $filename, + FilesystemOperator $storage, + ?Closure $onStreamComplete = null, + string $contentDisposition = HttpResponseHeaders::ATTACHMENT_TYPE->value, + ): StreamedResponse { + return $this->getFileStreamedResponse( + $path, + $mimeType, + $filename, + $storage, + $contentDisposition, + $onStreamComplete, + ); + } + }; + } +}