fix(api): stop reporting failed writes as HTTP 200 - #291
Open
HugoFara wants to merge 1 commit into
Open
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #284.
The defect
Handlers across the API report failure by returning
['error' => ...]or['success' => false, ...]rather than by throwing, and every router hands that return value straight toResponse::success(). Sent as-is it became HTTP 200 —fetchreportedok, the client took the payload as data, and the interface silently did nothing.This is what made #283 invisible:
POST /api/v1/terms/quickanswered 200, changed no rows, logged no error, and updated nothing on screen.The fix
Two halves.
Backend — the shape is recognised at the single chokepoint
Response::success()and given a 400, rather than rewriting the ~300 return sites. The body is passed through untouched, so frontend code already reading the message out of the payload keeps working; only the status becomes honest.Detection is deliberately narrow:
'error' => nullis what handlers emit on the way out of a success branch, so presence of the key means nothing — the value has to be a non-empty string (or a baretrueflag).'errors' => [...], a result field on batch endpoints, is not a failure signal. A caller that named its own status keeps it.Frontend — the API client only ever looked for
messageon a failed request, while the API sendserror. Even a correctly-formedResponse::error()therefore surfaced as a bareHTTP 400: Bad Request, with the real reason discarded. All six wrappers now share one extractor that reads both, plus the status line as a last resort.apiPostMultipartalready did this by hand; that copy is now the shared one.Why no call-site churn
I checked all 75 places the frontend reads an error out of an
ApiResponsebody. Every one of them already checksresponse.errorfirst (or falls back to it), so they now receive the more specific message rather than losing it. Theresponse.data.errorbranches become unreachable for promoted responses but are kept — endpoints that bypassResponse::success()(DictionaryControllerechoes JSON directly) still return errors inside a 200.Note for API consumers
This is a visible API change: endpoints that answered 200-with-an-error now answer 400. That is the point of the issue, but it is worth knowing for the Lukaisu client and any external consumer.
Verification
BookApiHandlerTest::routePostHandlesTheCollectionasserted 200 for what its own comment called "the handled-failure path" — it was documenting the defect. Updated to 400.tsc --noEmitclean, ESLint clean