Change representation of failed uploads (possible BC break)

FileUploadChunk has been unusable for most of the upload failures due to the missing content range and size information.
nepada · Jan 18, 2024 · 555c0ae · 555c0ae
1 parent b8ea50a
commit 555c0ae
Show file tree

Hide file tree

Showing 11 changed files with 168 additions and 33 deletions.
diff --git a/phpstan.neon.dist b/phpstan.neon.dist
@@ -69,7 +69,7 @@ parameters:
             path: src/FileUploadControl/FileUploadControl.php
         -
             message: "#^Parameter \\#2 \\$error of method Nepada\\\\FileUploadControl\\\\FileUploadControl\\:\\:createUploadErrorResponse\\(\\) expects string, mixed given\\.$#"
-            count: 2
+            count: 3
             path: src/FileUploadControl/FileUploadControl.php
         -
             message: "#^Variable property access on Nette\\\\Utils\\\\Html\\.$#"

diff --git a/src/FileUploadControl/FileUploadControl.php b/src/FileUploadControl/FileUploadControl.php
@@ -8,6 +8,7 @@
 use Nepada\FileUploadControl\Responses\UploadErrorResponse;
 use Nepada\FileUploadControl\Responses\UploadSuccessResponse;
 use Nepada\FileUploadControl\Storage\ContentRange;
+use Nepada\FileUploadControl\Storage\FailedUpload;
 use Nepada\FileUploadControl\Storage\FileUploadChunk;
 use Nepada\FileUploadControl\Storage\FileUploadId;
 use Nepada\FileUploadControl\Storage\FileUploadItem;
@@ -100,6 +101,10 @@ public function loadHttpData(): void
 
         $uploadFailed = false;
         foreach ($fileUploadChunks as $fileUploadChunk) {
+            if ($fileUploadChunk instanceof FailedUpload) {
+                $uploadFailed = true;
+                continue;
+            }
             try {
                 $storage->save($fileUploadChunk);
             } catch (UnableToSaveFileUploadException $exception) {
@@ -266,6 +271,11 @@ public function handleUpload(string $namespace): void
                 continue;
             }
 
+            if ($fileUploadChunk instanceof FailedUpload) {
+                $responses[] = $this->createUploadErrorResponse($fileUploadChunk, $this->translate('Upload error'));
+                continue;
+            }
+
             if ($fileUploadChunk->contentRange->containsFirstByte()) {
                 $fakeFileUpload = new FileUpload([
                     'name' => $fileUploadChunk->fileUpload->getUntrustedName(),
@@ -443,11 +453,11 @@ protected function createUploadSuccessResponse(FileUploadItem $fileUploadItem):
         );
     }
 
-    protected function createUploadErrorResponse(FileUploadChunk $fileUploadChunk, string $error): Response
+    protected function createUploadErrorResponse(FileUploadChunk|FailedUpload $upload, string $error): Response
     {
         return new UploadErrorResponse(
-            $fileUploadChunk->fileUpload->getUntrustedName(),
-            $fileUploadChunk->contentRange->getSize(),
+            $upload->fileUpload->getUntrustedName(),
+            $upload->contentRange?->getSize() ?? 0,
             $error,
         );
     }
@@ -475,7 +485,7 @@ protected function parseFileUploadId(string $value): FileUploadId
     }
 
     /**
-     * @return FileUploadChunk[]
+     * @return list<FileUploadChunk|FailedUpload>
      * @throws Nette\Application\BadRequestException
      */
     protected function getFileUploadChunks(): array
@@ -492,22 +502,24 @@ protected function getFileUploadChunks(): array
             if (count($files) > 1) {
                 throw new Nette\Application\BadRequestException('Chunk upload does not support multi-file upload', 400);
             }
+            $file = reset($files);
+
             try {
                 $contentRange = ContentRange::fromHttpHeaderValue($contentRangeHeaderValue);
-                $fileUploadChunk = FileUploadChunk::partialUpload(reset($files), $contentRange);
-                return [$fileUploadChunk];
+                $fileUpload = $file->isOk() ? FileUploadChunk::partialUpload($file, $contentRange) : FailedUpload::of($file, $contentRange);
+                return [$fileUpload];
             } catch (\Throwable $exception) {
                 throw new Nette\Application\BadRequestException('Invalid content-range header value', 400, $exception);
             }
         }
 
-        /** @var FileUploadChunk[] $fileUploadChunks */
-        $fileUploadChunks = [];
+        /** @var list<FileUploadChunk|FailedUpload> $fileUploads */
+        $fileUploads = [];
         foreach ($files as $file) {
-            $fileUploadChunks[] = FileUploadChunk::completeUpload($file);
+            $fileUploads[] = $file->isOk() ? FileUploadChunk::completeUpload($file) : FailedUpload::of($file);
         }
 
-        return $fileUploadChunks;
+        return $fileUploads;
     }
 
 }
diff --git a/src/FileUploadControl/Storage/FailedUpload.php b/src/FileUploadControl/Storage/FailedUpload.php
@@ -0,0 +1,36 @@
+<?php
+declare(strict_types = 1);
+
+namespace Nepada\FileUploadControl\Storage;
+
+use Nette;
+
+final class FailedUpload
+{
+
+    use Nette\SmartObject;
+
+    public readonly Nette\Http\FileUpload $fileUpload;
+
+    public readonly ?ContentRange $contentRange;
+
+    private function __construct(Nette\Http\FileUpload $fileUpload, ?ContentRange $contentRange)
+    {
+        if ($fileUpload->isOk()) {
+            throw new \InvalidArgumentException("Expected failed file upload, but upload of '{$fileUpload->getUntrustedName()}' has been successful.");
+        }
+
+        $this->fileUpload = $fileUpload;
+        $this->contentRange = $contentRange;
+    }
+
+    public static function of(Nette\Http\FileUpload $fileUpload, ?ContentRange $contentRange = null): self
+    {
+        if ($contentRange === null && $fileUpload->getSize() > 0) {
+            $contentRange = ContentRange::ofSize($fileUpload->getSize());
+        }
+
+        return new self($fileUpload, $contentRange);
+    }
+
+}
diff --git a/src/FileUploadControl/Storage/FileSystemStorage.php b/src/FileUploadControl/Storage/FileSystemStorage.php
@@ -90,10 +90,6 @@ public function delete(FileUploadId $id): void
      */
     public function save(FileUploadChunk $fileUploadChunk): FileUploadItem
     {
-        if (! $fileUploadChunk->fileUpload->isOk()) {
-            throw UnableToSaveFileUploadException::withUploadError();
-        }
-
         if ($fileUploadChunk->contentRange->containsFirstByte()) {
             $id = $this->saveNewUpload($fileUploadChunk);
         } else {

diff --git a/src/FileUploadControl/Storage/FileUploadChunk.php b/src/FileUploadControl/Storage/FileUploadChunk.php
@@ -16,6 +16,10 @@ final class FileUploadChunk
 
     private function __construct(Nette\Http\FileUpload $fileUpload, ContentRange $contentRange)
     {
+        if (! $fileUpload->isOk()) {
+            throw new \InvalidArgumentException("Expected successful file upload, but upload of '{$fileUpload->getUntrustedName()}' has failed with error {$fileUpload->getError()}.");
+        }
+
         if ($fileUpload->getSize() !== $contentRange->getRangeSize()) {
             throw new \InvalidArgumentException(sprintf(
                 'Content range size of %d does not match file upload size %d.',

diff --git a/src/FileUploadControl/Storage/UnableToSaveFileUploadException.php b/src/FileUploadControl/Storage/UnableToSaveFileUploadException.php
@@ -6,11 +6,6 @@
 class UnableToSaveFileUploadException extends \RuntimeException
 {
 
-    public static function withUploadError(): self
-    {
-        return new self('Unable to save file upload, because of pre-existing upload error.');
-    }
-
     public static function withConflict(FileUploadId $id): self
     {
         return new self("Unable to save file upload '{$id->toString()}', because of conflict with existing data.");

diff --git a/tests/FileUploadControl/FileUploadControlValidationTest.phpt b/tests/FileUploadControl/FileUploadControlValidationTest.phpt
@@ -32,6 +32,19 @@ require_once __DIR__ . '/../bootstrap.php';
 class FileUploadControlValidationTest extends TestCase
 {
 
+    public function testSubmittedWithFailedUpload(): void
+    {
+        $storage = InMemoryStorage::createWithFiles();
+        $control = $this->createFileUploadControl($storage);
+
+        $files = ['fileUpload' => ['upload' => [
+            FileUploadFactory::createFailed('fail'),
+        ]]];
+        $this->submitForm($control, $files);
+
+        Assert::same(['translated:Upload error'], $control->getErrors());
+    }
+
     public function testSubmittedRequiredValueMissing(): void
     {
         $control = $this->createFileUploadControl();
@@ -67,6 +80,18 @@ class FileUploadControlValidationTest extends TestCase
         Assert::same(['translated:max 1 upload allowed'], $control->getErrors());
     }
 
+    public function testUploadWithFailedUpload(): void
+    {
+        $control = $this->createFileUploadControl();
+
+        $files = ['fileUpload' => ['upload' => [
+            FileUploadFactory::createFailed('fail'),
+        ]]];
+        $this->doUpload($control, $files);
+
+        Assert::same('{"files":[{"name":"fail","size":0,"error":"translated:Upload error"}]}', $this->extractJsonResponsePayload($control));
+    }
+
     public function testUploadWithDisabledControl(): void
     {
         $control = $this->createFileUploadControl();

diff --git a/tests/FileUploadControl/FileUploadFactory.php b/tests/FileUploadControl/FileUploadFactory.php
@@ -41,4 +41,9 @@ public static function createWithSize(int $size, ?string $name = null): FileUplo
         return self::create($name, $size, 'tmp_name', UPLOAD_ERR_OK);
     }
 
+    public static function createFailed(string $name): FileUpload
+    {
+        return self::create($name, 0, 'tmp_name', UPLOAD_ERR_INI_SIZE);
+    }
+
 }
diff --git a/tests/FileUploadControl/Storage/FailedUploadTest.phpt b/tests/FileUploadControl/Storage/FailedUploadTest.phpt
@@ -0,0 +1,62 @@
+<?php
+declare(strict_types = 1);
+
+namespace NepadaTests\FileUploadControl\Storage;
+
+use Nepada\FileUploadControl\Storage\ContentRange;
+use Nepada\FileUploadControl\Storage\FailedUpload;
+use NepadaTests\FileUploadControl\FileUploadFactory;
+use NepadaTests\TestCase;
+use Tester\Assert;
+
+require_once __DIR__ . '/../../bootstrap.php';
+
+
+/**
+ * @testCase
+ */
+class FailedUploadTest extends TestCase
+{
+
+    public function testCreateWithKnownContentRange(): void
+    {
+        $fileUpload = FileUploadFactory::create('name', 0, 'tmp', UPLOAD_ERR_NO_TMP_DIR);
+        $contentRange = ContentRange::fromHttpHeaderValue('bytes 0-9/42');
+        $failedUpload = FailedUpload::of($fileUpload, $contentRange);
+        Assert::same($fileUpload, $failedUpload->fileUpload);
+        Assert::same($contentRange, $failedUpload->contentRange);
+    }
+
+    public function testCreateWithContentRangeDerivedFromSize(): void
+    {
+        $fileUpload = FileUploadFactory::create('name', 42, 'tmp', UPLOAD_ERR_PARTIAL);
+        $failedUpload = FailedUpload::of($fileUpload);
+        Assert::same($fileUpload, $failedUpload->fileUpload);
+        Assert::notNull($failedUpload->contentRange);
+        Assert::same(42, $failedUpload->contentRange->getSize());
+    }
+
+    public function testCreateWithoutContent(): void
+    {
+        $fileUpload = FileUploadFactory::create('name', 0, 'tmp', UPLOAD_ERR_NO_TMP_DIR);
+        $failedUpload = FailedUpload::of($fileUpload);
+        Assert::same($fileUpload, $failedUpload->fileUpload);
+        Assert::null($failedUpload->contentRange);
+    }
+
+    public function testOkFileUploadIsRejected(): void
+    {
+        $fileUpload = FileUploadFactory::create('name', 42, 'tmp', UPLOAD_ERR_OK);
+        Assert::exception(
+            function () use ($fileUpload): void {
+                FailedUpload::of($fileUpload);
+            },
+            \InvalidArgumentException::class,
+            "Expected failed file upload, but upload of 'name' has been successful.",
+        );
+    }
+
+}
+
+
+(new FailedUploadTest())->run();
diff --git a/tests/FileUploadControl/Storage/FileSystemStorageTest.phpt b/tests/FileUploadControl/Storage/FileSystemStorageTest.phpt
@@ -34,19 +34,6 @@ class FileSystemStorageTest extends TestCase
         Assert::same([], $storage->list());
     }
 
-    public function testNotOkFileUploadIsRejected(): void
-    {
-        $storage = $this->createStorage();
-        $chunk = FileUploadChunk::completeUpload(FileUploadFactory::create('name', 42, 'tmp', UPLOAD_ERR_PARTIAL));
-        Assert::exception(
-            function () use ($storage, $chunk): void {
-                $storage->save($chunk);
-            },
-            UnableToSaveFileUploadException::class,
-            'Unable to save file upload, because of pre-existing upload error.',
-        );
-    }
-
     public function testAttemptToResumeUploadOfOrphanedMetadataFails(): void
     {
         $metadata = FileUploadMetadata::fromArray(['name' => 'orphaned.txt', 'size' => 42]);

diff --git a/tests/FileUploadControl/Storage/FileUploadChunkTest.phpt b/tests/FileUploadControl/Storage/FileUploadChunkTest.phpt
@@ -55,6 +55,19 @@ class FileUploadChunkTest extends TestCase
         );
     }
 
+    public function testNotOkFileUploadIsRejected(): void
+    {
+        $fileUpload = FileUploadFactory::create('name', 0, 'tmp', UPLOAD_ERR_INI_SIZE);
+        $contentRange = ContentRange::fromHttpHeaderValue('bytes 0-9/42');
+        Assert::exception(
+            function () use ($fileUpload, $contentRange): void {
+                FileUploadChunk::partialUpload($fileUpload, $contentRange);
+            },
+            \InvalidArgumentException::class,
+            "Expected successful file upload, but upload of 'name' has failed with error 1.",
+        );
+    }
+
 }