diff --git a/src/Infrastructure/Adapter/In/Api/Controllers/AccountFile/UploadController.php b/src/Infrastructure/Adapter/In/Api/Controllers/AccountFile/UploadController.php index f6463a0cc..7774e1013 100644 --- a/src/Infrastructure/Adapter/In/Api/Controllers/AccountFile/UploadController.php +++ b/src/Infrastructure/Adapter/In/Api/Controllers/AccountFile/UploadController.php @@ -60,11 +60,21 @@ public function uploadAction(): ApiResponse ); } + $name = $this->apiService->getParamString('name', true); + $fileData = new File([ 'accountId' => $accountId, - 'name' => $this->apiService->getParamString('name', true), + 'name' => $name, 'type' => $resolvedType, - 'extension' => $this->apiService->getParamString('extension'), + // The extension is documented as optional and the column is NOT NULL, so leaving it + // out sent null to the database and came back as HTTP 500 "Integrity constraint" with + // a raw SQLSTATE in the detail — the parameter was required in everything but name. + // It is derived from the file name instead, which is what the web upload has always + // done, so the two paths now store the same thing for the same file. + 'extension' => self::extensionFrom( + $this->apiService->getParamString('extension'), + $name + ), 'size' => strlen($content), 'content' => $content, ]); @@ -86,4 +96,20 @@ public function uploadAction(): ApiResponse $id ); } + + /** + * The extension to store: the caller's, or the one the file name carries. + * + * Uppercased to match the web upload, which stores `mb_strtoupper(pathinfo(…))` — the same + * file arriving by either route should not end up recorded two different ways. + * + * A name with no extension yields an empty string rather than null. The column is NOT NULL + * with no default, and an empty extension is a fact about the file; null is a failed insert. + */ + private static function extensionFrom(?string $given, string $name): string + { + $given = trim((string)$given); + + return mb_strtoupper($given !== '' ? $given : pathinfo($name, PATHINFO_EXTENSION)); + } } diff --git a/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/AccountFile/UploadControllerTest.php b/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/AccountFile/UploadControllerTest.php index 164a2f792..88f2df73a 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/AccountFile/UploadControllerTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Api/Controllers/AccountFile/UploadControllerTest.php @@ -7,6 +7,8 @@ use PHPUnit\Framework\Attributes\Group; use SP\Domain\Core\Acl\AclActionsInterface; use SP\Tests\Integration\Infrastructure\Adapter\In\Api\ApiTestCase; + +use function SP\Tests\getDbHandler; use stdClass; /** @@ -85,6 +87,26 @@ private function upload(int $accountId, array $params): stdClass )); } + /** + * The extension as it was actually stored. + * + * Read from the row rather than from the upload's own response, which echoes back only the id + * and the name — an assertion against what the endpoint just told us would not show what the + * database now holds, which is the thing that was wrong. + */ + private function extensionStoredFor(int $fileId): string + { + $statement = getDbHandler()->getConnection() + ->prepare('SELECT `extension` FROM `AccountFile` WHERE `id` = :id'); + $statement->execute(['id' => $fileId]); + + $extension = $statement->fetchColumn(); + + $this->assertNotFalse($extension, sprintf('No AccountFile row with id %d', $fileId)); + + return (string)$extension; + } + // ----------------------------------------------------------------------- // Tests // ----------------------------------------------------------------------- @@ -107,6 +129,73 @@ public function testValidUpload(): void $this->assertGreaterThan(0, $r->body->itemId); } + /** + * The extension is documented as optional, so leaving it out has to work. + * + * It did not: the column is NOT NULL, so an omitted extension reached the database as null and + * the caller got HTTP 500 `Integrity constraint`, with `SQLSTATE[23000] … Column 'extension' + * cannot be null` in the detail — an internal database error handed to an API client for using + * the endpoint as documented. Every test here passed one, which is why nothing noticed. + */ + public function testAnUploadWithoutAnExtensionTakesItFromTheName(): void + { + $accountId = $this->createAccount(); + + $r = $this->upload($accountId, [ + 'name' => 'notes.txt', + 'content' => base64_encode(self::PLAIN_TEXT_CONTENT), + 'type' => 'text/plain', + ]); + + $this->assertSame(200, $r->status, 'an optional parameter must be optional'); + $this->assertSame('File uploaded', $r->body->message); + + $this->assertSame( + 'TXT', + $this->extensionStoredFor($r->body->itemId), + 'the name says .txt, and the web upload would have stored TXT for the same file' + ); + } + + /** + * A name with no extension stores an empty one rather than failing. + * + * Empty is a fact about the file — plenty of real attachments have no extension — while null + * is an insert the database refuses. + */ + public function testAnUploadOfAFileWithNoExtensionIsStored(): void + { + $accountId = $this->createAccount(); + + $r = $this->upload($accountId, [ + 'name' => 'README', + 'content' => base64_encode(self::PLAIN_TEXT_CONTENT), + 'type' => 'text/plain', + ]); + + $this->assertSame(200, $r->status); + $this->assertSame('', $this->extensionStoredFor($r->body->itemId)); + } + + /** + * An extension the caller does give is still the one used, and is normalised the way the web + * upload normalises it. + */ + public function testAnExtensionTheCallerGivesIsKept(): void + { + $accountId = $this->createAccount(); + + $r = $this->upload($accountId, [ + 'name' => 'notes.txt', + 'content' => base64_encode(self::PLAIN_TEXT_CONTENT), + 'type' => 'text/plain', + 'extension' => 'log', + ]); + + $this->assertSame(200, $r->status); + $this->assertSame('LOG', $this->extensionStoredFor($r->body->itemId)); + } + public function testOversizedUploadIsRejected(): void { $accountId = $this->createAccount();