Skip to content

Handler error payloads are wrapped in Response::success(), so failed writes return HTTP 200 #284

Description

@sarahmccuan

Describe the bug

Several API handlers signal failure by returning ['error' => ...], but the router passes that array straight to Response::success(). The client receives HTTP 200 with an error key buried in the body, so a failed write is indistinguishable from a successful one and the UI silently does nothing.

Observed while marking a word known (the underlying failure is #283):

POST /api/v1/terms/quick  ->  200
  no PHP error logged, no rows changed, UI does not update

Cause

VocabularyApiRouter::routePost():

} elseif ($frag1 === 'quick') {
    return Response::success($this->termHandler->formatQuickCreate(
        (int) ($params['text_id'] ?? 0),
        (int) ($params['position'] ?? 0),
        (int) ($params['status'] ?? 0)
    ));
}

TermCrudApiHandler::createQuickTerm():

try {
    $result = $this->discoveryService->insertWordWithStatus($textId, $term, $status);
    return ['term_id' => $result['id'], ...];
} catch (\RuntimeException $e) {
    return ['error' => $e->getMessage()];
}

DatabaseException extends LwtException extends RuntimeException, so real database errors — duplicate key, constraint violations, connection failures — are caught by that handler, flattened into an error array, and then wrapped in a success response.

This is not confined to quick. In the same router, for-edit, for-language, full, bulk and multi all pass a handler that can return ['error' => ...] into Response::success(...). getTermForEdit() alone has three such returns ('Term not found', 'Text not found', 'Language not found'), each of which currently reaches the client as HTTP 200.

Worth noting the inconsistency: terms/full has no equivalent catch, so the identical database failure escapes as a 500 with a visible error. Same defect, two different symptoms, depending only on which endpoint the UI happens to call — which makes this class of bug hard to diagnose from the client side.

Suggested fix

Handlers should throw, or return a typed result the router can distinguish; the router should map an error outcome onto a 4xx/5xx rather than Response::success(). A narrower stopgap would be for the router to inspect the returned array for an error key before choosing the response helper.

Note on verification

Traced from the code together with server logs and database state (200 response, no error logged, no row modified). I did not reproduce it with a direct request — CSRF validation rejected a replayed curl, and I did not want to bypass it against live data.

Server

  • LWT 3.4.2-fork (v003004002)
  • PHP 8.4.24, MariaDB 12.1.2, Apache 2.4.68 (Debian), Docker on Windows

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions