Isolate the response decoder from auth and transport in Kotlin and Swift

Merged
JE
2mo ago
+1096-114

Closes #604

Kotlin and Swift ran encode → URL build → auth → transport → status check →
decode inside one try/do per request primitive, with a terminal catch that
mapped nothing. A malformed 2xx body therefore surfaced as the decoder's own
exception — indistinguishable, to a caller catching BasecampException /
BasecampError, from the auth strategy throwing or the socket dropping.

This wraps the decode expression in each primitive, mapping a decode failure
to the SPEC §6 statusless api_error shape that the §18 composites already
produced by hand. It therefore covers all 250 operations, not just the six
composites.

mapped typeresult
KotlinSerializationException (incl. MissingFieldException)BasecampException.Api(httpStatus = null, retryable = false, cause = e)
SwiftDecodingError, plus the CocoaError JSONSerialization raises on the wrapped-list path for a body that is not JSON at allBasecampError.api(httpStatus: nil) → isRetryable == false; underlying error interpolated into message

BasecampError.api gained no cause slot: adding an associated value would be a
compile break for every switch over the case (Class C), and the migration entry
for this change is Class B. Swift carries the underlying error in the message,
exactly as its composites already did.

Wrap the expression, not the block — and why

The generated Kotlin services serialize the request body inside the fn
lambda (json.encodeToString(...), see any generated mutation), which runs
inside the same try as the decode and throws the very same
SerializationException type. Wrapping the block would relabel a
request-encoding fault as a malformed response — the original conflation in a
new shape. Swift has the same positional hazard with try Self.encoder.encode
before the transport call, with a different error type.

So the mapping is positional, not type-based, and three negative tests per
language pin it: an auth-strategy throw, a transport failure and a request-body
encoding failure must all still surface exactly as before.

Decode sites touched

Kotlin — kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/services/BaseService.kt, 7 sites, all now decodeOrApiError(info.operation) { … }:

  • request — the single-object parse
  • requestPaginated — first page, and the follow-loop page
  • requestPaginatedWrapped — first page, and the follow-loop page
  • requestPaginatedAsFlow — first page, and the follow-loop page

Swift — swift/Sources/Basecamp/Services/BaseService.swift, 6 sites, all now Self.decoding(operation) { … }:

  • request — decoder.decode(T.self, …)
  • requestPaginated — decoder.decode([T].self, …)
  • requestPaginatedWrapped → decodeWrappedItems
  • followPagination — the loop's decoder.decode
  • followWrappedPagination → decodeWrappedItems
  • decodeWrappedItems itself — all three throwing steps (JSONSerialization.jsonObject, JSONSerialization.data, decoder.decode) are the decode of that response and nothing else

requestVoid has no decode site. The two private follow*Pagination helpers took
an operation parameter so the message can name the operation, as Kotlin's does.

The catch blocks: kept and adapted, not deleted

The issue proposed deleting the composites' catch (e: SerializationException) /
catch let error as DecodingError blocks as redundant. They are not
subsumed, and deleting them would have changed observable behaviour:

  • Kotlin asserts the composite-specific message —
    TodolistsServiceTest.kt:415,416,482,602 require "does not decode as a todolist",
    which the generic helper cannot produce.
  • Both languages assert the composite-specific hint naming the escape hatch —
    DocumentsServiceTest.kt:110, SchedulesServiceTest.kt:123,
    TodolistsServiceTest.kt:338,374,434; DocumentsServiceExtensionsTests.swift:148,
    SchedulesServiceExtensionsTests.swift:316, TodolistsServiceExtensionsTests.swift:614.

Leaving them untouched was not an option either: once the base layer maps the
decode failure, catch (SerializationException) / catch as DecodingError never
fires again, and the hint would have been lost silently. They now key off the
wrapped form and pass everything else through:

  • Kotlin — catch (e: BasecampException.Api) with e.cause as? SerializationException ?: throw e
  • Swift — catch let error as BasecampError, asking BaseService.malformedBodyMessage

That Swift helper exists because statuslessness alone does not identify a
malformed body: the pagination same-origin guard throws a statusless .api
too. The first version of the conformance-runner change inferred it from the
shape and reported both cross-origin security.json cases as "Mock body lacks
required Swift model fields" — so the SDK now writes the identifying phrase once
and reads it back through one function, which the composites ask instead of
inferring.

The runner asks that same function, over @_spi

The Kotlin runner matches structurally — (e as? BasecampException.Api)?.cause as? SerializationException — and the compiler holds it. Swift has no cause to match,
so the first version of the runner carried its own copy of the phrase, checked by
nothing but a comment at each end. The SDK test that pins the phrase does not
close that: a rename goes red in the SDK suite, where the obvious repair is to
update the test — leaving the runner silently stale. A gate that points away from
the broken thing is worse than none.

BaseService.malformedBodyMessage is now @_spi(Conformance) public, and the
runner reaches it with @_spi(Conformance) import Basecamp. One literal, in one
module, and the link is a symbol reference the compiler checks. SPI rather than
plain public because the answer is only meaningful to a caller that already
knows how this SDK renders the failure; it is additive either way.

Proof that the coupling now follows a rename instead of drifting past it —
malformedBodyPhrase changed to "sent back something the model refuses", no
other edit:

$ ( set -o pipefail; make conformance-swift > spi-rename.log 2>&1 ); echo "REAL_EXIT=$?" >> spi-rename.log
Passed: 193, Failed: 0, Skipped: 1, Total: 194
REAL_EXIT=0

Before the SPI that rename would have left the runner matching a phrase the SDK
no longer emits, and every decoder rejection would have been misfiled as a real
api_error — silently, since no fixture asserts the difference.

Existing tests that pinned the old contract

Seven tests asserted the decoder's own exception escaping the SDK — the contract
this replaces. Each keeps its original question and asks it of the new shape:
Kotlin's two (CardsServiceTest, UpcomingScheduleDecodeTest) assert the
statusless Api and that the decoder's exception survives as cause; Swift's
five match the coding path in the message instead, because .api has no cause
slot — a shared assertStatuslessDecodeFailure helper does the shape half.

Swift's five cannot go back to structured DecodingError matching, and exposing
malformedBodyMessage does not change that: the mapping renders the error to a
string at the point it is caught, so nothing structural survives it in any
caller, SPI or not. Recovering it means the cause slot this PR declines to add.
What each of those tests was actually asserting — which field the decoder
refused — is still asserted, because the coding path is in the interpolated
description.
GeneratedServiceTests.testDecodeErrorFromMalformedJSON had a catch-all fallback
that accepted any error at all, so it asserted nothing; it is now tightened.

Conformance runners: the #555 policy re-applied at a new seam

Both decoder-backed runners caught BasecampException/BasecampError before
their SerializationException/DecodingError arms, so with the mapping in place
a decode failure would have been routed to the wrong branch. Two things would
have silently changed:

  • the loud Mock body lacks required Kotlin/Swift model fields: … failure — the
    #555 stop-on-mismatch policy that keeps an under-specified fixture body from
    degrading coverage quietly — would never fire again;
  • caughtError/caughtException is deliberately withheld for a decoder
    rejection so that a fixture pinning errorCode cannot be satisfied by one.
    Routing it onward would have handed it over.

Both runners now recognise the mapped error at that seam and apply exactly the
old policy: Kotlin off the SerializationException cause (so
MissingFieldException still gets its own message), Swift off the
malformed-body phrase — not the statusless shape, which the same-origin guard
also has, as the security.json failure above proved. Their original arms stay
live for the one decode the primitives do not own — the generated wrapper-field
decode described below.

Red proof

Positive tests, against the pre-fix code at 9aef1ccf3 (source files restored
with git show, never git stash — the stash stack is shared across worktrees):

$ ( set -o pipefail; ./gradlew :basecamp-sdk:jvmTest --tests "com.basecamp.sdk.DecodeIsolationTest" > kt-red.log 2>&1 ); echo "REAL_EXIT=$?" >> kt-red.log
DecodeIsolationTest[jvm] > malformedBodyOnASingleGetIsAStatuslessApiError()[jvm] FAILED
DecodeIsolationTest[jvm] > malformedWrappedBodyIsAStatuslessApiError()[jvm] FAILED
DecodeIsolationTest[jvm] > malformedSecondPageIsAStatuslessApiError()[jvm] FAILED
DecodeIsolationTest[jvm] > malformedFirstPageIsAStatuslessApiError()[jvm] FAILED
7 tests completed, 4 failed
REAL_EXIT=1

$ ( set -o pipefail; swift test --filter DecodeIsolationTests > sw-red.log 2>&1 ); echo "REAL_EXIT=$?" >> sw-red.log
testMalformedBodyOnASingleGetIsAStatuslessApiError : failed - expected BasecampError, got a raw
  DecodingError: DecodingError.keyNotFound: Key 'appUrl' not found …
testMalformedWrappedBodyIsAStatuslessApiError : failed - expected BasecampError, got a raw
  NSError: Error Domain=NSCocoaErrorDomain Code=3840 "JSON text did not start with array or object…"
Executed 7 tests, with 4 failures (0 unexpected)
REAL_EXIT=1

Green on this branch: REAL_EXIT=0, 7/7 in each language.

The negative tests do not go red against the pre-fix code, and saying so is the
point. Pre-fix, an auth or encode failure already surfaced raw; what they guard
is the wrong-shaped fix. Proven by mutation:

# Kotlin mutation 1 — the primitive's own catch maps SerializationException
#   ("wrapped the block", narrow type)
DecodeIsolationTest[jvm] > aRequestBodyEncodingFailureIsNotAnApiError()[jvm] FAILED
7 tests completed, 1 failed
REAL_EXIT=1

# Kotlin mutation 2 — the terminal catch maps every Exception ("wrapped the block", widened)
DecodeIsolationTest[jvm] > aRequestBodyEncodingFailureIsNotAnApiError()[jvm] FAILED
DecodeIsolationTest[jvm] > anAuthStrategyFailureIsNotAnApiError()[jvm] FAILED
7 tests completed, 2 failed
REAL_EXIT=1

# Swift mutation — terminal catch maps every error to malformedBody
testAnAuthStrategyFailureIsNotAnApiError failed
testARequestBodyEncodingFailureIsNotAnApiError failed
testATransportFailureIsNotAnApiError failed
Executed 7 tests, with 3 failures (0 unexpected)
REAL_EXIT=1

Under every mutation the four positive tests still pass — which is exactly why the
negative ones have to exist.

Verification

$ ( set -o pipefail; make > make4.log 2>&1 ); echo "REAL_EXIT=$?" >> make4.log
All checks passed!
==> All checks passed
REAL_EXIT=0

Two holes review found in the mapping itself

Not every decode failure is a SerializationException. Person.id decodes
through FlexibleLongSerializer, which reaches JsonPrimitive.long —
content.toLong() — so an unquoted fractional or out-of-range id raised
NumberFormatException and escaped the mapping entirely, on nearly every
response the API can send.

Fixed at the point the wrong type is raised, not at the catch. The mapped error's
cause is a contract three composites and the conformance runner all read
structurally (cause is SerializationException); a second mapped type would have
left composites rethrowing without their escape-hatch hint and let the runner
satisfy an errorCode: api_error assertion with a decoder rejection. The
serializer now translates, keeping the numeric original beneath, and
decodeOrApiError still catches exactly one type.

A 204 for a value-returning operation never reached the decoder. The
primitive short-circuited any 204 to Unit as T, and the unchecked cast raises
nothing at the boundary — so the operation succeeded, handing back Unit wearing
the model's type, with the ClassCastException arriving later at whatever site
first used the value, or never if the caller discarded it. The red proof shows it
as "completed successfully", which is the sharper indictment. The
shortcut is deleted rather than guarded: all 48 void operations the generator
emits parse with { Unit }, which ignores the body, so letting an empty body
reach the decoder answers them correctly and makes the value-returning case the
malformed response it is.

Both are red against their own mutation — dropping the serializer's translation,
and restoring the 204 shortcut — each failing exactly the new tests and nothing
else.

Known gap, deliberately not closed here

GetPersonProgress decodes its wrapper after the primitive returns, in
generated code in both SDKs — the person member, plus the events array, which
Kotlin reaches through ["events"]!!.jsonArray. Those failures still surface raw
(DecodingError, NullPointerException, IllegalArgumentException), and
Swift's absent events is not refused at all: it decodes to an empty list, where
Kotlin throws. One operation, flagged rather than half-fixed — it needs both
service generators plus a regeneration, the !! failures are NPEs that a decode
mapping should not be catching, and whether an absent events is malformed at
all is a contract question needing the bc3 template.

Tracked in #728, which carries the full table of what leaks where. SPEC.md §6
and MIGRATING.md now both record the exception, so the documented contract
matches the code rather than promising what this PR declines to deliver.

Also

  • MIGRATING.md — Class B entry for Kotlin and Swift under Unreleased.
  • SPEC.md §6 — the statusless-api_error section said this shape belongs to
    composites; it now records that the typed SDKs reach it on every operation, and
    that the mapping is expression-scoped.