Skip to content

Fix HTTPDataResponse.success error handling and converge failure reporting - #34

Merged
germ-mark merged 2 commits into
mainfrom
mark/fix-success-error-handling
Jul 28, 2026
Merged

Fix HTTPDataResponse.success error handling and converge failure reporting#34
germ-mark merged 2 commits into
mainfrom
mark/fix-success-error-handling

Conversation

@germ-mark

@germ-mark germ-mark commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Both success overloads wrapped the status check and the result decode in one do, so a decode failure on a 2xx fell into the catch and got re-decoded as the error type. Status now selects the branch and decode failures propagate.

Behavior changes

Three, all visible to oauth4swift — the only consumer.

  • 2xx whose body doesn't decode as the result type. Was: returned .error(ErrorResponse, 200), so processGenericAccessToken reported a malformed token response as Errors.invalidRequest — an OAuth protocol error for what is really a decode failure. Now: throws the real DecodingError for the result type.
  • Non-2xx whose body doesn't decode as the error type (proxy HTML, empty body). Was: bare DecodingError, status and bytes lost. Now: HTTPResponseError.unsuccessful(code, data).
  • Every failure path now reports .unsuccessful. expectSuccess() alone reported .unsuccessfulString whenever the body happened to be UTF-8, so two near-identically-named methods disagreed for the same response — and an empty body arrived as .unsuccessfulString(code, ""), since String(data: Data(), encoding: .utf8) is "" rather than nil. .unsuccessfulString stays in the enum so existing callers compile; it is simply no longer thrown from this module.

HTTPResponseError gains code and bodyString, which read either case — that is the migration path for anyone who was matching .unsuccessfulString. Nothing in oauth4swift, AtprotoOAuth, AtprotoClient or germDM catches an HTTPResponseError thrown by an HTTPDataResponse method today; the three sites that name .unsuccessfulString use the type as a carrier for their own throws.

Note code can carry a 2xx: expect(statusCode: 201) on a 200 reports .unsuccessful(200, …). That is longstanding behavior of expect(statusCode:), and success(code:) now matches it rather than diverging — worth knowing before branching on code.

Tests

First coverage this type has had. I established which tests are genuine regression tests by reverting only the two success bodies while keeping every addition: three fail, and they are marked as such in the file. The rest pin existing behavior and say so, including one whose comment previously implied otherwise.

That distinction depends on a fixture that decodes as the error type but not the result type — a naive "not json" body throws DecodingError under both implementations and proves nothing.

Also covered: success(code:) against a real failure status, a bodiless 2xx (documents a sharp edge rather than endorsing it — success always has a result type to decode, so callers with no success body want #35's expectSuccess(orError:)), and agreement across all three failure paths.

Test notes

swift test green. Also ran oauth4swift against this branch via swift package edit: it compiles, its suite passes, and a throwaway harness over processPushedAuthorizationResponse, processGenericAccessToken and both discovery call sites confirmed the unchanged paths still behave and the changed ones improve. oauth4swift has no tests of its own over those paths, and this reaches AtprotoOAuth by transitive resolution as soon as it releases — that harness was the only gate, so it is worth a standing test in oauth4swift eventually.

🤖 Generated with Claude Code

Both success overloads wrapped the status check and the result decode in one
do, so a 2xx whose body failed to decode as R was re-decoded as E - returning
a bogus .error for a successful response, or masking the real DecodingError.
Status now selects the branch and decode failures propagate.

A failure whose body does not decode as E throws HTTPResponseError.unsuccessful
rather than a bare DecodingError, keeping the status and raw bytes. Adds code
and bodyString accessors so callers can read either case without matching both.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 60f06df

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@germ-network/germ-convenience Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

expectSuccess() reported .unsuccessfulString whenever the body happened to be
UTF-8, while expect(statusCode:) and the new decodedError reported .unsuccessful
for the same response - so two near-identically named methods disagreed, and an
empty body arrived as .unsuccessfulString(code, "") since String(data: Data())
is "" rather than nil.

.unsuccessfulString stays in the enum so existing callers compile; it is just no
longer thrown from here. bodyString reads either case.

Also covers success(code:) against a real failure status, a bodiless 2xx, and
corrects a test comment that implied it pinned the fix when it passes against
the old implementation too.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@germ-mark germ-mark changed the title Fix HTTPDataResponse.success error handling Fix HTTPDataResponse.success error handling and converge failure reporting Jul 28, 2026
@germ-mark
germ-mark merged commit 175d392 into main Jul 28, 2026
10 checks passed
@germ-mark
germ-mark deleted the mark/fix-success-error-handling branch July 28, 2026 20:26
@github-actions github-actions Bot mentioned this pull request Jul 28, 2026

@ThisIsMissEm ThisIsMissEm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense to me

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants