Skip to content

refactor(kit): APIError discards structured error bodies, so 400/409 details are unreachable #103

Description

@Adron

Surfaced twice in one day, from opposite ends of the codebase. Filing it once.

The gap

APIError keeps only the decoded {error} string from a failed response. Everything else in the body is discarded at APIClient.swift:234, before any caller sees it:

case 400: return .badRequest(serverMessage: serverMessage)

So an error body that carries structured information — the thing the client needs in order to do anything but show a sentence — cannot reach the code that would act on it.

Two concrete callers, both already written around it

1. The list-schema destructive-change guard (PR #87, closing #85). PUT /api/lists/{id}/schema refuses to drop a column that still holds row data, answering 400 with a propertiesWithData array naming the columns. That is a question, not a malfunction — the UI should name the columns and offer the confirmation. ListSchemaConflictDTO models the full shape and nothing can populate it, so the user gets the server's sentence and no column list.

2. The app-settings compare-and-set conflict (PR #102, closing #56). The app-settings family is compare-and-set: a stale baseVersion answers 409 with the current document attached, so the client can show what changed, or merge, or re-base and retry. Today it can only report that a conflict happened.

Neither is a hypothetical. Both are shipped code with a comment explaining why the better behaviour is absent.

Why it has not been fixed in passing

APIError.badRequest(serverMessage: String?) and its siblings carry an associated value that every case .badRequest(let message) in the codebase pattern-matches. Adding a second associated value breaks every one of those sites, so it is not a change to slip into a feature PR — which is exactly why both PRs flagged it and moved on.

Suggested shape

Keep the body alongside the message rather than replacing it:

case badRequest(serverMessage: String?, body: Data?)

…with the existing pattern matches updated, or — less invasive — a parallel accessor that does not change the cases:

extension APIError {
    /// The raw response body, when the failure carried one.
    var responseBody: Data? { … }

    /// Decodes the body as `T`, or `nil` when it was absent or did not match.
    func details<T: Decodable>(as type: T.Type) -> T?
}

The second keeps every call site compiling and gives both callers what they need. Worth weighing which reads better before writing it — the first is more honest about where the data lives, the second is far cheaper.

Acceptance

  • ListsService.updateSchema surfaces the propertiesWithData column names, and the schema editor's confirmation names them.
  • AppSettingsService surfaces the current document from a 409.
  • A decode failure on an error body degrades to today's behaviour — a message with no details — rather than masking the original error.
  • BDD quartet on the accessor: a body that decodes, a body that does not, an error with no body at all, and a non-HTTP failure.

Activity

Adron commented on Sep 16, 2026

@Adron
MemberAuthor

Implemented in PR #106. Work continues on the PR.

The design is opt-in, for the reason this issue anticipated: adding an associated value to APIError's cases is a mechanical change to 420 sites across 78 files, including every test, in service of two call sites.

sendCapturingFailure(_:) throws an APIFailure carrying the body; every other entry point keeps throwing plain APIError and not one of those sites changes. There is a test pinning exactly that property, because it is the one an implementation could break silently.

details(as:) returns nil rather than throwing on a mismatch — the acceptance criterion this issue names. A caller asking for details is asking an optional question, and a decode failure there must not mask the HTTP failure being handled.

One thing worth knowing in review

Raising the richer error inside the transport silently disabled the 401 session-retry and the whole retry policy — performWithSafetyNet and performWithRetry both catch let error as APIError, which stopped matching. The existing AuthTransportTests caught it. Both now match on underlyingError and rethrow the APIFailure whole, because flattening there would drop the body before sendCapturingFailure ever saw it.

The two consumers are deliberately not wired here

ListsService.updateSchema lives on PR #87 and AppSettingsService on PR #102. Wiring them in this PR would mean duplicating those branches or building a three-way stack.

Once #87, #102 and #106 have merged, each is a one-line change — swap api.send for api.sendCapturingFailure and read details(as:) — and ListSchemaConflictDTO already exists for the first. Worth doing as a small follow-up rather than leaving the accessor unused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions