Bring GetPersonProgress's wrapper decode inside the request primitive

Merged
JE
2mo ago
+795-153

#604 isolated the response decoder in every Kotlin and Swift request primitive:
a body that does not decode becomes SPEC §6's statusless, non-retryable
api_error. GetPersonProgress was the one operation that escaped it, because
the half of its envelope that is not the items array was decoded by generated
code running after the primitive returned. It is the SDK's only
wrapped-pagination operation (itemsKey: appears once in Swift generated code;
]!! appeared exactly twice in all of kotlin/**/generated/, both on that
method), and it was the last [manual] carve-out in §6.

It is two SDKs, not six: Go returns decode errors verbatim at its *WithResponse
sites, and TypeScript, Ruby and Python never typed-decode the
wrapper at all. The sibling GetUpcomingSchedule decodes typed inside the
primitive and was already safe — verified, untouched.

Closes #728.

The contract question: both keys are required

bc3's app/views/api/users/timelines/show.json.jbuilder, in full:

json.person @person, partial: "people/person", as: :person
json.events @events, partial: "timelines/events/event", as: :event

No conditional. Users::TimelinesController#set_person is a before_action
that finds or 404s, and #show assigns @events for every api_request?.
Both keys are always on the wire, so an absent one is malformed, not empty.

That settles both halves and makes the two SDKs agree:

  • Swift's guard let itemsArray = json[key] else { return [] } was wrong — and
    so was the as? [String: Any] ?? [:] non-object fallback. Both are now
    refusals.
  • Kotlin's !! was right in intent, wrong in type. It is no longer emitted; NPE
    is not caught (mapping it would swallow genuine programming errors).

The fix is generator-side

Both service emitters changed, then make generate re-emitted the services:

beforeafter
Kotlin itemsparseToJsonElement(body).jsonObject["events"]!!.jsonArray.map { … }decodeFromJsonElement<List<TimelineEvent>>(decodeFromString<JsonObject>(body).requiredMember("events"))
Kotlin wrapperdecodeFromJsonElement<Person>(wrapper["person"]!!)decodeFromJsonElement<Person>(wrapper.requiredMember("person"))
Swift items?? [:] + else { return [] }DecodingError.typeMismatch / .keyNotFound / .typeMismatch
Swift wrappertry Self.decoder.decode(Wrapper.self, …) after the primitive returnedthe same decode, inside the primitive's decoding(_:_:)

requiredMember is a new internal extension in
com.basecamp.sdk.serialization — no public surface — whose only job is to make
absence speak SerializationException, the one type decodeOrApiError maps.

The Kotlin visibility decision, and why Swift got the same shape

decodeOrApiError is private and the generated ReportsService is a
subclass, so the person half could not be wrapped without either widening it
to protected or threading a decode lambda through requestPaginatedWrapped.
(The events half needed neither — the items lambda already runs inside the
isolation.)

Decision: keep decodeOrApiError private and thread the lambda. Reasoning,
taken fresh rather than inherited from #772:

  • The two options cost the same surface. protected on a public open
    class is public API; so is changing an existing protected signature. There
    is no cheap option, so cost does not decide it.
  • They buy different guarantees. A protected helper is a guarantee the
    generated code has to opt into every time someone adds a wrapped operation.
    Threading the lambda means there is no way to reach the wrapper body at all
    without passing through the mapping — the next wrapped operation inherits it.
  • Swift did not strictly need a visibility change (decoding is internal
    and generated services are in-module), so symmetry alone would not have
    justified it. What did: requestPaginatedWrapped is public and was handing
    raw Data back out of a decode-isolating primitive. Removing that from the
    public surface is an improvement on its own terms.
  • Ordering fell out of it. finishWrapped in both SDKs is the single exit,
    because the wrapper decode must run before onOperationEnd fires. Otherwise
    a malformed wrapper is reported to hooks as a success and then thrown — which
    is precisely what happened while the decode lived in generated code.

Source break for anyone subclassing BaseService directly (generated services
are its only callers here); documented in MIGRATING.

Red proof — per case, both SDKs, with the regenerate step

A mutation that only edits a generator proves nothing, so the harness reverts
the four hand-edited files to HEAD, regenerates, and only then runs.
Restore is cp + diff -q throughout — never git checkout --, never
git stash.

Regeneration under the pre-fix generators put the old accessors back, verified
before the run:

--- emitted Kotlin items/wrapper accessors:
156:            json.parseToJsonElement(body).jsonObject["events"]!!
162:            person = json.decodeFromJsonElement<Person>(wrapper["person"]!!)
--- emitted Swift wrapper decode:
70:        let (wrapperData, items): (Data, ListResult<TimelineEvent>) = try await requestPaginatedWrapped(
81:        let wrapper = try Self.decoder.decode(Wrapper.self, from: wrapperData)

RED — Kotlin (REAL_EXIT=1, 16 tests completed, 5 failed) — note the
exception class per case, all of them raw:

aNonObjectWrappedBodyIsAStatuslessApiError()  FAILED
    Caused by: java.lang.IllegalArgumentException
aNonArrayItemsKeyIsAStatuslessApiError()      FAILED
    Caused by: java.lang.IllegalArgumentException
aWrongTypedWrapperMemberIsAStatuslessApiError() FAILED
    Caused by: kotlinx.serialization.json.JsonDecodingException
anAbsentWrapperMemberIsAStatuslessApiError()  FAILED
    Caused by: java.lang.NullPointerException
anAbsentItemsKeyIsAStatuslessApiError()       FAILED
    Caused by: java.lang.NullPointerException

RED — Swift (REAL_EXIT=1, 16 tests, 6 failures / 3 unexpected). The
events case is the sharp one: it did not throw the wrong error, it did not
throw at all.

testAnAbsentItemsKeyIsAStatuslessApiError : failed - expected a malformed wrapper to fail
testAnAbsentItemsKeyIsAStatuslessApiError : XCTAssertTrue failed - the absent member must be named, got:
testANonObjectWrappedBodyIsAStatuslessApiError : failed: caught error: "DecodingError.typeMismatch: Expected value of type Dictionary<String, Any> … but found an array instead."
testAnAbsentWrapperMemberIsAStatuslessApiError : failed: caught error: "DecodingError.keyNotFound: Key 'person' not found …"
testAWrongTypedWrapperMemberIsAStatuslessApiError : failed: caught error: "DecodingError.typeMismatch: … Path: person … but found number instead."
testANonArrayItemsKeyIsAStatuslessApiError : XCTAssertTrue failed - the wrong-typed member must be named

(testANonObjectWrappedBodyIsAStatuslessApiError and
testAnAbsentItemsKeyIsAStatuslessApiError are the two that used to return
events: [] silently; the first surfaces here only because the wrapper decode,
still outside the isolation, then threw on person.)

Restore, regenerate with the fixed generators, and the regenerated output was
byte-identical to the committed tree (diff -q clean on both emitted files).

GREEN — Kotlin BUILD SUCCESSFUL, REAL_EXIT=0.
GREEN — Swift Executed 16 tests, with 0 failures, REAL_EXIT=0.

Cases covered, in both SDKs: wrapper missing events; wrapper missing person;
person of the wrong type; events non-array; non-object top-level body; plus
a well-formed control that still decodes.

Carve-out deleted

  • SPEC.md — the paragraph beginning "The primitive is the boundary, and
    one decode currently falls outside it.    " is gone. One sentence was added to
    the paragraph above it, stating the contract the spec never carried (an
    absent envelope member is a malformed body, not an empty result) rather than
    the mechanism, which lives in the two SDKs' own comments.
  • MIGRATING.md — "One operation is exempt, and it is GetPersonProgress"
    and the forward reference to it are gone, replaced by what a caller has to do:
    Swift's silent empty list is a behaviour change, and
    requestPaginatedWrapped changed shape for subclassers.

The section keeps its [manual] tag, deliberately. [conformance] claims
the conformance suite verifies it, and the suite is cross-SDK by construction.
This shape exists only where a typed decoder does; Go, TypeScript, Ruby and
Python would each need a skip for a malformed-wrapper case, which is exactly
what SPEC §19's zero-skip roster exists to stop. The evidence stays in per-SDK
unit tests, where #604 put it. Nothing else keys off the tag — §19's roster is
generated from spec/zero-skip-roster.yml and was not touched.

make check-fixture-coverage wanted nothing: 33 covered schemas, 51 manifest
targets, all accounted for. No new conformance suite was needed —
GetPersonProgress already has dispatch in all six runners via paths.json.

CI-only failure, fixed in 82914765d

Two of the new Swift assertions matched on 'events', apostrophes included.
That passed on my Swift 6.4 and failed on CI with both SDKs behaving
identically: 6.4 has a CustomStringConvertible for DecodingError and prints
the nested debugDescription verbatim, while the CI toolchain reflects the enum
and escapes the inner string's apostrophes to \'. The assertion was pinned to
the renderer, not the behaviour. It now checks the member name and the complaint
separately, which survive either rendering — and mutating the two
debugDescriptions in BaseService to "vanished" / "is wrong" fails both
tests, so the replacement is not vacuous.

The Conformance Tests job failed only as the downstream aggregator of that job.

Review round 1 — four findings, all taken (f3d1b6419)

  • The Smithy model contradicted the SPEC sentence this PR adds.
    GetPersonProgressOutput.person/.events were still optional, so
    openapi.json had no required array and the four SDKs that never
    typed-decode this envelope described always-present members as optional. Both
    are @required now; 7 files regenerated. Two hand-written sites followed the
    generated types rather than being patched around: timeline.go dropped its
    != nil branch (and still returns *Person, so no Go caller breaks), and
    PublicAPIConsumer moved off the zero-argument init the model no longer has.
    The one real cost, named rather than hidden: Go stops distinguishing an absent
    person from a zero-valued one, which is a choice between two wrong answers on
    a body BC3 cannot produce, and Go never errored on it either way.
  • finishWrapped's ordering was documented and untested — hoisting the hook
    above the decode would have passed the whole file. Both SDKs now assert exactly
    one onOperationEnd carrying the mapped error for a malformed wrapper and
    exactly one success for a well-formed one. Verified by mutation: the hoist
    fails that test and only that test, in each SDK.
  • Swift's requestPaginatedWrapped doc still described the removed contract
    on a public method. Rewritten.

Review round 2 — three suppressed findings, all taken (e1895b6e6)

Copilot suppressed three comments on the second pass. All three were correct
about the code, and all three were about my prose rather than the change:

  • The Kotlin test comment said both halves of the envelope decoded after the
    primitive returned. Only person did — parseItems has always run inside
    decodeOrApiError, and events surfaced raw for the other reason, the
    unmapped NullPointerException/IllegalArgumentException. A boundary problem
    and an exception-type problem; that distinction is why the fix has two halves.
  • The Swift comment on the non-array case had the mechanism backwards. {} is
    valid input to data(withJSONObject:), so that case was already an
    api_error and the guard adds a message naming the member. What the guard
    genuinely prevents is a string or number at that key, which
    data(withJSONObject:) answers with an uncatchable
    NSInvalidArgumentException.
  • GetPersonProgressResponseContent was still sitting in the all-optional
    roster with a comment saying it had left it. Moved to
    requiredMemberModelsKeepTheirInitShape; roster count 35 → 34.

One correction that lands on the red proof above: of the five malformed
shapes, the non-array events case is a genuine before/after in Kotlin only
(raw IllegalArgumentException → statusless api_error). In Swift it was
already an api_error; what changed there is the message. The other four are
genuine before/after in both SDKs, and the two silent-empty-list cases are
Swift-only by construction.

Review round 3 — five suppressed findings, all taken (76c9c93a1)

  • "The four SDKs that never typed-decode this envelope" is wrong about Go.
    ParseGetPersonProgressResponse unmarshals straight into
    GetPersonProgressResponseContent. Go typed-decodes; what it lacks is any
    notion of a required member, because encoding/json has none. Only
    TypeScript, Ruby and Python skip the typed decode. Fixed in the Smithy comment
    and MIGRATING, which both used the wrong grouping to justify @required.
  • MIGRATING repeated the boundary-vs-exception-type conflation and said a
    wrong-typed person leaked an NPE — it leaked a SerializationException; the
    NPE was the !! on an absent one. Split into the two mechanisms.
  • MIGRATING understated the @required break. Not only construction and not
    only Go/Swift: TypeScript object literals, Python TypedDict literals, and
    Go/Swift != nil / if let / reassignment all break. Now a table, one row per
    SDK, including Ruby's "nothing".
  • The non-array test did not exercise the crash it was documented against.
    {} is valid input to data(withJSONObject:); a scalar is not, and reaches an
    NSInvalidArgumentException that is not a Swift error and cannot be caught.
    Added testAScalarItemsKeyIsRefusedRatherThanCrashing, verified the only way
    it can be: deleting the type guard does not fail the test, it terminates the
    runner with an uncaught NSException.

Review round 4 — one overstatement, in four places (223ae6783)

Copilot's third pass found the same error four times, and it is right. Only
one malformed wrapper used to come back as a success: an absent events
alongside a valid person. A non-object top-level body, and a non-array
events, reached the same ?? [:] / return [] fallback — but the wrapper
decode rejected the body a line later, so those operations did fail, with a
raw DecodingError about person, naming the wrong member.

That distinction was already visible in the red proof above (the non-object case
threw before the fix) and in the fixtures (the absent-events case carries a
well-formed person precisely so the silence is attributable to the items key),
but MIGRATING, decodeWrappedItems' doc and two test comments all said "silent
empty list" for all of them. Corrected. The earlier Swift commit message carries
the overstatement too, and 223ae6783 is the correction of record.

Review round 5 — and a change of instrument (7c644129f)

Two more, both true: Kotlin's absent person raised an NPE from
wrapper["person"]!!, not the SerializationException a wrong-typed one
raised; and a non-array events never reached Swift's empty-list fallback —
{} serialized fine and the array decoder already produced a mapped
api_error, while a scalar aborted the process.

That is four rounds correcting the same paragraph, which is a signal about the
instrument rather than a queue of fixes: prose was flattening six bodies × two
SDKs into one sentence and losing a distinction every time. It is a table
now, one row per body, two columns of prior behaviour — and every cell was
produced by running the tests against the pre-change generators and primitives,
not reasoned about. That turned up one cell no test covered, so the Kotlin suite
gains aScalarItemsKeyIsAStatuslessApiError; against the base commit it fails
with the IllegalArgumentException the table claims.

Net for the red proof: 18 Kotlin tests, 7 failing against base (was 5 —
the scalar case and the hook-ordering case joined), and 439 Swift tests green,
with testAScalarItemsKeyIsRefusedRatherThanCrashing provably crashing the
runner if its guard is deleted.

Gates

gatelocaleexit
make kt-checkLC_ALL=C0
make kt-check-driftLC_ALL=C0 (no drift)
make swift-checkLC_ALL=C0 (431 → 437 tests)
make doc-constants-checkLC_ALL=C0 (27 marked spans across 7 files)
make check-fixture-coverageLC_ALL=C0
make check (full, incl. conformance)LC_ALL=C.UTF-80 (re-run green after the review round)

Locale note: the full make check ran under LC_ALL=C.UTF-8, not LC_ALL=C,
because the conformance runner is broken under LC_ALL=C — that is #774 and is
unrelated to this change. Every gate that does not go through the runner was run
under LC_ALL=C as well, as the table shows.