[PR #44] [MERGED] refactor(auth): extract shared OAuth token-endpoint POST helper #46

New issue

Closed

opened 2026-10-26 22:34:21 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:34:21 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-swift-sdk/pull/44
Author: @mattmillerai
Created: 7/11/2026
Status: ✅ Merged
Merged: 7/16/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-2861-oauth-token-endpoint

📝 Commits (2)

e8c5e44 refactor(auth): extract shared OAuth token-endpoint POST helper (BE-2861)
0cf459b fix(auth): x-www-form-urlencoded token body; drop unused tokenType

📊 Changes

4 files changed (+126 additions, -89 deletions)

View changed files

📝 Sources/ComfySwiftSDK/Internal/Auth/OAuthExchanger.swift (+7 -42)
➕ Sources/ComfySwiftSDK/Internal/Auth/OAuthTokenEndpoint.swift (+86 -0)
📝 Sources/ComfySwiftSDK/Internal/Auth/OAuthTokenRefreshExecutor.swift (+7 -47)
📝 Tests/ComfySwiftSDKTests/OAuthExchangeTests.swift (+26 -0)

📄 Description

ELI-5

Two bits of the OAuth code — the one that trades an authorization code for tokens (OAuthExchanger) and the one that refreshes an expired token (OAuthTokenRefreshExecutor) — were doing the exact same five-step "POST a form to the token endpoint and decode the reply" dance, copy-pasted. This PR pulls that shared dance into one helper (OAuthTokenEndpoint.post) so there's a single copy. Each caller still builds its own form fields; nothing about how the SDK behaves changes.

What changed

New Sources/ComfySwiftSDK/Internal/Auth/OAuthTokenEndpoint.swift — an internal enum with a single static func post(queryItems:session:mapAuthInvalidToExpired:) that owns the shared flow: form-encode the body → POST application/x-www-form-urlencoded → session.data(for:) in a do/catch → Transport.translate → Transport.checkStatus → decode the token DTO → build OAuthTokenResponse.
OAuthExchanger.exchange now builds only its query items and delegates to the helper with mapAuthInvalidToExpired: false. Its private TokenExchangeDTO is deleted.
OAuthTokenRefreshExecutor.refresh now builds only its query items and delegates with mapAuthInvalidToExpired: true. Its private TokenRefreshDTO is deleted.
Both actors, their init(session:), and every call site are untouched.

Net: +87 / −89, a pure refactor with no public API and no behavior change.

Load-bearing differences preserved (each pinned by an existing test)

authInvalid → authExpired remap stays refresh-only. The helper wraps checkStatus in catch ComfyError.authInvalid where mapAuthInvalidToExpired. Exchange passes false, so its .authInvalid propagates unchanged; refresh passes true, so a rejected refresh surfaces .authExpired. The where clause guarantees any non-authInvalid error (rate-limited, network, etc.) propagates in both modes.
Optional token_type. The merged TokenDTO keeps tokenType: String? so both the exchange mock body (which includes token_type) and the refresh mock body (which omits it) decode.
Guarded body encoding. The helper adopts refresh's stricter form: a nil form body throws ComfyError.unknown(underlying: URLError(.badURL)) rather than silently sending an empty body (exchange's old behavior). Strictly safer; in practice unreachable for a non-empty query-item list.

Note for the reviewer

The current source had already evolved to thread per-client config into these methods (exchange(..., config: OAuthClientConfig = .comfyIOS) and refresh(..., clientId:) with its RFC 6749 §6 comment). Those parameters only affect how each caller builds its query items, not the shared five-step dance, so they were preserved verbatim and the extraction applies cleanly on top.

Testing

Full package suite green locally — 226 tests, 0 failures (swift build + swift test). The four behavior-pinning tests called out for this change all pass:

exchange HTTP 401 throws .authInvalid
second 401 after a successful refresh surfaces .authExpired, never a loop
refresh failure (401 on the refresh POST — family revoked) surfaces .authExpired
apiKey mode 401 still surfaces .authInvalid (no refresh machinery) — regression

No new tests added: behavior is unchanged and already fully covered (the optional guarded-body .badURL path is unreachable through the public query-item API, so a test would only assert the guard's own premise).

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-swift-sdk/pull/44 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/11/2026 **Status:** ✅ Merged **Merged:** 7/16/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-2861-oauth-token-endpoint` --- ### 📝 Commits (2) - [`e8c5e44`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/e8c5e4412b8472429fab087630c10392e018810d) refactor(auth): extract shared OAuth token-endpoint POST helper (BE-2861) - [`0cf459b`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/0cf459bc7e95d97d89ce2f571bbc8bad0e31e9cf) fix(auth): x-www-form-urlencoded token body; drop unused tokenType ### 📊 Changes **4 files changed** (+126 additions, -89 deletions) <details> <summary>View changed files</summary> 📝 `Sources/ComfySwiftSDK/Internal/Auth/OAuthExchanger.swift` (+7 -42) ➕ `Sources/ComfySwiftSDK/Internal/Auth/OAuthTokenEndpoint.swift` (+86 -0) 📝 `Sources/ComfySwiftSDK/Internal/Auth/OAuthTokenRefreshExecutor.swift` (+7 -47) 📝 `Tests/ComfySwiftSDKTests/OAuthExchangeTests.swift` (+26 -0) </details> ### 📄 Description ## ELI-5 Two bits of the OAuth code — the one that trades an authorization code for tokens (`OAuthExchanger`) and the one that refreshes an expired token (`OAuthTokenRefreshExecutor`) — were doing the exact same five-step "POST a form to the token endpoint and decode the reply" dance, copy-pasted. This PR pulls that shared dance into one helper (`OAuthTokenEndpoint.post`) so there's a single copy. Each caller still builds its own form fields; nothing about how the SDK behaves changes. ## What changed - New `Sources/ComfySwiftSDK/Internal/Auth/OAuthTokenEndpoint.swift` — an internal `enum` with a single `static func post(queryItems:session:mapAuthInvalidToExpired:)` that owns the shared flow: form-encode the body → POST `application/x-www-form-urlencoded` → `session.data(for:)` in a `do/catch` → `Transport.translate` → `Transport.checkStatus` → decode the token DTO → build `OAuthTokenResponse`. - `OAuthExchanger.exchange` now builds only its query items and delegates to the helper with `mapAuthInvalidToExpired: false`. Its private `TokenExchangeDTO` is deleted. - `OAuthTokenRefreshExecutor.refresh` now builds only its query items and delegates with `mapAuthInvalidToExpired: true`. Its private `TokenRefreshDTO` is deleted. - Both actors, their `init(session:)`, and every call site are untouched. Net: **+87 / −89**, a pure refactor with **no public API and no behavior change**. ## Load-bearing differences preserved (each pinned by an existing test) 1. **`authInvalid` → `authExpired` remap stays refresh-only.** The helper wraps `checkStatus` in `catch ComfyError.authInvalid where mapAuthInvalidToExpired`. Exchange passes `false`, so its `.authInvalid` propagates unchanged; refresh passes `true`, so a rejected refresh surfaces `.authExpired`. The `where` clause guarantees any *non*-`authInvalid` error (rate-limited, network, etc.) propagates in both modes. 2. **Optional `token_type`.** The merged `TokenDTO` keeps `tokenType: String?` so both the exchange mock body (which includes `token_type`) and the refresh mock body (which omits it) decode. 3. **Guarded body encoding.** The helper adopts refresh's stricter form: a nil form body throws `ComfyError.unknown(underlying: URLError(.badURL))` rather than silently sending an empty body (exchange's old behavior). Strictly safer; in practice unreachable for a non-empty query-item list. ## Note for the reviewer The current source had already evolved to thread per-client config into these methods (`exchange(..., config: OAuthClientConfig = .comfyIOS)` and `refresh(..., clientId:)` with its RFC 6749 §6 comment). Those parameters only affect **how each caller builds its query items**, not the shared five-step dance, so they were preserved verbatim and the extraction applies cleanly on top. ## Testing Full package suite green locally — **226 tests, 0 failures** (`swift build` + `swift test`). The four behavior-pinning tests called out for this change all pass: - `exchange HTTP 401 throws .authInvalid` - `second 401 after a successful refresh surfaces .authExpired, never a loop` - `refresh failure (401 on the refresh POST — family revoked) surfaces .authExpired` - `apiKey mode 401 still surfaces .authInvalid (no refresh machinery) — regression` No new tests added: behavior is unchanged and already fully covered (the optional guarded-body `.badURL` path is unreachable through the public query-item API, so a test would only assert the guard's own premise). --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:34:21 +00:00