From 2bee015d89dc0099d8597730a01a639a2c2db03c Mon Sep 17 00:00:00 2001 From: HugoFara Date: Thu, 27 Aug 2026 15:03:06 +0200 Subject: [PATCH] fix(api): stop reporting failed writes as HTTP 200 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Handlers across the API report a failure by returning `['error' => ...]` or `['success' => false, ...]` rather than by throwing, and every router hands that return value straight to `Response::success()`. Sent as-is it became HTTP 200: `fetch` reported `ok`, the client took the payload as data, and the interface silently did nothing. Recognise the shape at that one chokepoint and give it a 400 instead of rewriting 300-odd return sites. The body is passed through untouched, so the frontend code that reads the message out of the payload keeps working — only the status becomes honest. `'error' => null` is what handlers emit on the way out of a *success* branch, so the value has to be a non-empty message (or a bare `true` flag) to count. The API client was the other half. It only ever looked for `message` on a failed request, while the API sends `error`, so even a correctly-formed error response surfaced as a bare "HTTP 400: Bad Request". All six wrappers now share one extractor that reads both. BookApiHandlerTest asserted 200 for what its own comment called "the handled-failure path" — it was documenting the defect, and now expects 400. Fixes #284 --- CHANGELOG.md | 14 ++++ src/backend/Api/V1/Response.php | 58 +++++++++++++ src/frontend/js/shared/api/client.ts | 69 +++++++-------- tests/backend/Api/V1/ResponseTest.php | 78 +++++++++++++++++ .../Modules/Book/Http/BookApiHandlerTest.php | 8 +- tests/frontend/core/api_client.test.ts | 84 +++++++++++++++++++ 6 files changed, 268 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 282a17a8a..85cc40c17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,20 @@ ones are marked like "v1.0.0-fork". ## [Unreleased] +### Fixed + +* **A failed API write no longer reports success** (#284). Handlers signal + failure by returning an `error` payload rather than by throwing, and the + routers passed that straight to `Response::success()` — so the request came + back HTTP 200 and the interface, seeing a success, did nothing at all. Such + a payload is now recognised and sent as 400. The body is unchanged, so + anything reading the message out of it keeps working. + +* **API error messages reach the interface** (#284). The API client only ever + looked for `message` on a failed request, while the API sends `error` — so + even a well-formed error was shown as a bare "HTTP 400: Bad Request". It now + reads both. + ## [3.5.0-fork] - 2026-08-27 ### Added diff --git a/src/backend/Api/V1/Response.php b/src/backend/Api/V1/Response.php index ae0f09760..0a4923a7f 100644 --- a/src/backend/Api/V1/Response.php +++ b/src/backend/Api/V1/Response.php @@ -26,6 +26,16 @@ */ class Response { + /** + * Status given to a handler payload that reports a failure. + * + * 400 rather than a shape-specific code: the payloads carry a message but + * no indication of *why* they failed, and guessing 404 from wording would + * break as soon as a message is translated. A handler that knows better + * should call {@see self::error()} or {@see self::notFound()} directly. + */ + private const FAILURE_STATUS = 400; + /** * Create JSON response. * @@ -42,6 +52,19 @@ public static function send(int $status, mixed $data): JsonResponse /** * Create success response. * + * Many handlers report a failure by *returning* `['error' => ...]` or + * `['success' => false, ...]` rather than by throwing, and every router + * hands that return value straight to this method. Sent as-is it becomes + * HTTP 200, so a failed write is indistinguishable from a successful one: + * `fetch` reports `ok`, the client takes the payload as data, and the UI + * silently does nothing (issue #284 — observed on POST /terms/quick, where + * the underlying duplicate-key failure of #283 was invisible). + * + * Rather than rewrite 300-odd return sites, the shape is recognised here + * and given a 4xx status. The body is left exactly as the handler built + * it, so anything already reading `error` out of the payload keeps working + * — only the status becomes honest. + * * @param mixed $data Response data * @param int $status HTTP status code (default 200) * @@ -49,9 +72,44 @@ public static function send(int $status, mixed $data): JsonResponse */ public static function success(mixed $data, int $status = 200): JsonResponse { + // Only the default is promoted: a caller that named a status meant it. + if ($status === 200 && self::signalsFailure($data)) { + $status = self::FAILURE_STATUS; + } + return self::send($status, $data); } + /** + * Does this payload report a failure rather than carry a result? + * + * `'error' => null` is common on the *success* branch of handlers that + * always include the key, so presence alone means nothing — the value has + * to be a non-empty message (or a bare `true` flag) to count. + * + * @param mixed $data Payload a handler returned + * + * @return bool True when the payload describes a failure + */ + private static function signalsFailure(mixed $data): bool + { + if (!is_array($data)) { + return false; + } + + if (array_key_exists('success', $data) && $data['success'] === false) { + return true; + } + + /** @var mixed $error */ + $error = $data['error'] ?? null; + if (is_string($error)) { + return trim($error) !== ''; + } + + return $error === true; + } + /** * Create error response. * diff --git a/src/frontend/js/shared/api/client.ts b/src/frontend/js/shared/api/client.ts index 9cc7a9db9..20fd09884 100644 --- a/src/frontend/js/shared/api/client.ts +++ b/src/frontend/js/shared/api/client.ts @@ -481,6 +481,28 @@ async function parseResponse(response: Response): Promise { } } +/** + * Pull a human-readable message out of a failed response. + * + * The API is not consistent about where the text lives. `Response::error()` + * emits `{error: "..."}`, some handlers return `{error: "...", success: false}` + * of their own, and the exception handler emits `{error: true, message: "..."}` + * — so `error` is only usable as the message when it is a string. Falling all + * the way through to the status line keeps a message on screen either way. + * + * Reading `error` at all is new: every wrapper below used to look only at + * `message`, which meant that even a correctly-formed error response arrived + * in the UI as a bare "HTTP 400: Bad Request". + */ +async function errorMessageFrom(response: Response): Promise { + const body = await parseResponse<{ message?: unknown; error?: unknown }>(response); + const fromError = typeof body.error === 'string' ? body.error : ''; + const fromMessage = typeof body.message === 'string' ? body.message : ''; + return ( + fromError || fromMessage || `HTTP ${response.status}: ${response.statusText}` + ); +} + /** * Make a GET request to the API. * @@ -505,12 +527,7 @@ export async function apiGet( }); if (!response.ok) { - const errorData = await parseResponse<{ message?: string }>(response); - return { - error: - errorData.message || - `HTTP ${response.status}: ${response.statusText}` - }; + return { error: await errorMessageFrom(response) }; } const data = await parseResponse(response); @@ -542,12 +559,7 @@ export async function apiPost( }); if (!response.ok) { - const errorData = await parseResponse<{ message?: string }>(response); - return { - error: - errorData.message || - `HTTP ${response.status}: ${response.statusText}` - }; + return { error: await errorMessageFrom(response) }; } const data = await parseResponse(response); @@ -579,12 +591,7 @@ export async function apiPut( }); if (!response.ok) { - const errorData = await parseResponse<{ message?: string }>(response); - return { - error: - errorData.message || - `HTTP ${response.status}: ${response.statusText}` - }; + return { error: await errorMessageFrom(response) }; } const data = await parseResponse(response); @@ -618,12 +625,7 @@ export async function apiDelete( const response = await apiFetch(defaultConfig.baseUrl + endpoint, options); if (!response.ok) { - const errorData = await parseResponse<{ message?: string }>(response); - return { - error: - errorData.message || - `HTTP ${response.status}: ${response.statusText}` - }; + return { error: await errorMessageFrom(response) }; } const data = await parseResponse(response); @@ -654,17 +656,7 @@ export async function apiPostMultipart( }); if (!response.ok) { - const errorData = await parseResponse<{ message?: unknown; error?: unknown }>(response); - // An error envelope may carry `error: true` alongside the real text in - // `message`, so only a string is usable as the message itself. - const fromError = typeof errorData.error === 'string' ? errorData.error : ''; - const fromMessage = typeof errorData.message === 'string' ? errorData.message : ''; - return { - error: - fromError || - fromMessage || - `HTTP ${response.status}: ${response.statusText}` - }; + return { error: await errorMessageFrom(response) }; } const data = await parseResponse(response); @@ -704,12 +696,7 @@ export async function apiPostForm( }); if (!response.ok) { - const errorData = await parseResponse<{ message?: string }>(response); - return { - error: - errorData.message || - `HTTP ${response.status}: ${response.statusText}` - }; + return { error: await errorMessageFrom(response) }; } const respData = await parseResponse(response); diff --git a/tests/backend/Api/V1/ResponseTest.php b/tests/backend/Api/V1/ResponseTest.php index c9b19ce12..c995d8682 100644 --- a/tests/backend/Api/V1/ResponseTest.php +++ b/tests/backend/Api/V1/ResponseTest.php @@ -61,6 +61,84 @@ public function testSuccessWithCustomStatus(): void $this->assertEquals(201, $response->getStatusCode()); } + /** + * A payload that reports a failure must not be sent as 200. + */ + public function testSuccessPromotesAnErrorPayloadToFourHundred(): void + { + $data = ['error' => 'Duplicate entry']; + $response = Response::success($data); + + $this->assertEquals(400, $response->getStatusCode()); + // The body is passed through untouched: callers already reading the + // message out of the payload keep finding it there. + $this->assertEquals($data, $response->getData()); + } + + /** + * The `success => false` convention is recognised too. + */ + public function testSuccessPromotesAnUnsuccessfulPayloadToFourHundred(): void + { + $response = Response::success(['success' => false, 'error' => 'Nope']); + + $this->assertEquals(400, $response->getStatusCode()); + } + + /** + * `error => null` is what handlers emit on the way *out* of a success + * branch, so it must stay a 200. + */ + public function testSuccessKeepsTwoHundredWhenTheErrorKeyIsEmpty(): void + { + foreach ([null, '', ' '] as $empty) { + $response = Response::success(['success' => true, 'error' => $empty]); + + $this->assertEquals( + 200, + $response->getStatusCode(), + 'error => ' . var_export($empty, true) . ' is not a failure' + ); + } + } + + /** + * An error envelope may flag failure with `error => true` and carry the + * text in `message`. + */ + public function testSuccessPromotesABooleanErrorFlag(): void + { + $response = Response::success(['error' => true, 'message' => 'Boom']); + + $this->assertEquals(400, $response->getStatusCode()); + } + + /** + * A caller that named its own status meant it. + */ + public function testSuccessLeavesAnExplicitStatusAlone(): void + { + $response = Response::success(['error' => 'Duplicate entry'], 201); + + $this->assertEquals(201, $response->getStatusCode()); + } + + /** + * Ordinary payloads are untouched, including non-arrays. + */ + public function testSuccessLeavesOrdinaryPayloadsAtTwoHundred(): void + { + $this->assertEquals(200, Response::success(['id' => 1])->getStatusCode()); + $this->assertEquals(200, Response::success([])->getStatusCode()); + $this->assertEquals(200, Response::success('plain')->getStatusCode()); + $this->assertEquals(200, Response::success(null)->getStatusCode()); + // "errors" is a result field on batch endpoints, not a failure flag. + $this->assertEquals( + 200, + Response::success(['imported' => 5, 'errors' => ['line 3']])->getStatusCode() + ); + } + /** * Test error returns JsonResponse with error format. */ diff --git a/tests/backend/Modules/Book/Http/BookApiHandlerTest.php b/tests/backend/Modules/Book/Http/BookApiHandlerTest.php index 2083edb39..3adaa8c0e 100644 --- a/tests/backend/Modules/Book/Http/BookApiHandlerTest.php +++ b/tests/backend/Modules/Book/Http/BookApiHandlerTest.php @@ -642,10 +642,14 @@ public function routeDeleteErrorMessageMentionsBookId(): void public function routePostHandlesTheCollection(): void { // No language in the body, so this is the handled-failure path rather - // than the trait's old 405 — POST /books is a real endpoint now. + // than the trait's old 405 — POST /books is a real endpoint now, and + // 400 rather than 405 is what distinguishes the two. It answered 200 + // until issue #284: the handler reports the missing language by + // returning an error payload, which Response::success() used to send + // as a success. $response = $this->handler->routePost([], []); - $this->assertSame(200, $response->getStatusCode()); + $this->assertSame(400, $response->getStatusCode()); } // ========================================================================= diff --git a/tests/frontend/core/api_client.test.ts b/tests/frontend/core/api_client.test.ts index 31787f7c8..5e2d77e3d 100644 --- a/tests/frontend/core/api_client.test.ts +++ b/tests/frontend/core/api_client.test.ts @@ -734,4 +734,88 @@ describe('core/api_client.ts', () => { expect(result.error).toBeDefined(); }); }); + // =========================================================================== + // Error message extraction (issue #284) + // =========================================================================== + + describe('error message extraction', () => { + it('reads the message from an "error" key', async () => { + // Response::error() emits {error: "..."}, and every wrapper used to look + // only at "message" — so a correctly-formed error arrived as the bare + // status line and the real reason never reached the UI. + mockFetch.mockResolvedValue({ + ok: false, + status: 400, + statusText: 'Bad Request', + text: () => Promise.resolve('{"error": "Duplicate entry"}') + }); + + const result = await apiPost('/terms/quick', {}); + + expect(result.error).toBe('Duplicate entry'); + expect(result.data).toBeUndefined(); + }); + + it('reads the message from a handler payload promoted to 400', async () => { + mockFetch.mockResolvedValue({ + ok: false, + status: 400, + statusText: 'Bad Request', + text: () => + Promise.resolve('{"success": false, "error": "No entries found"}') + }); + + const result = await apiPost('/local-dictionaries/import-curated', {}); + + expect(result.error).toBe('No entries found'); + }); + + it('prefers "message" when "error" is a bare flag', async () => { + mockFetch.mockResolvedValue({ + ok: false, + status: 500, + statusText: 'Internal Server Error', + text: () => Promise.resolve('{"error": true, "message": "Boom"}') + }); + + const result = await apiGet('/test'); + + expect(result.error).toBe('Boom'); + }); + + it('falls back to the status line when neither key is a string', async () => { + mockFetch.mockResolvedValue({ + ok: false, + status: 403, + statusText: 'Forbidden', + text: () => Promise.resolve('{"error": {"nested": "shape"}}') + }); + + const result = await apiGet('/test'); + + expect(result.error).toBe('HTTP 403: Forbidden'); + }); + + it('applies to every verb', async () => { + const body = '{"error": "Nope"}'; + for (const call of [ + () => apiGet('/t'), + () => apiPost('/t', {}), + () => apiPut('/t', {}), + () => apiDelete('/t'), + () => apiPostForm('/t', {}) + ]) { + mockFetch.mockResolvedValue({ + ok: false, + status: 400, + statusText: 'Bad Request', + text: () => Promise.resolve(body) + }); + + const result = await call(); + + expect(result.error).toBe('Nope'); + } + }); + }); });