From 97089247fa2a589548d8d98b9ee3a76b3370cb46 Mon Sep 17 00:00:00 2001 From: blaipr Date: Mon, 17 Aug 2026 20:30:16 +0200 Subject: [PATCH] Make the API's optional file extension actually optional MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `accountFile/upload` declares `extension` optional in its own help. Omitting it answered HTTP 500: {"error":{"message":"Integrity constraint", "detail":"SQLSTATE[23000]: ... Column 'extension' cannot be null"}} `AccountFile.extension` is `varchar(10) NOT NULL` with no default, the repository does not filter nulls out of the insert, and a null model property overrides a column's default — so using the endpoint exactly as documented handed an API client a raw SQLSTATE. Two things kept it invisible. Every one of the sixteen existing tests passes an `extension`, so the optional path was never exercised. And `ApiHelpMatchesControllersTest` passes, correctly: the help and the controller agree with each other about the parameter being optional. They both disagree with the database. The extension is now derived from the file name when it is not given, which is what the web upload has always done (`mb_strtoupper(pathinfo(...))`), so the same file arriving by either route is recorded the same way. A name carrying no extension stores an empty string — a fact about the file — rather than a null the database refuses. The new tests read the stored row rather than the upload's own response, which echoes back only the id and the name and so cannot show what was persisted. --- .../AccountFile/UploadController.php | 30 ++++++- .../AccountFile/UploadControllerTest.php | 89 +++++++++++++++++++ 2 files changed, 117 insertions(+), 2 deletions(-) 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();