diff --git a/App/Features/AI/AIDocumentSheet.swift b/App/Features/AI/AIDocumentSheet.swift index bd9ba96..cb19b29 100644 --- a/App/Features/AI/AIDocumentSheet.swift +++ b/App/Features/AI/AIDocumentSheet.swift @@ -22,7 +22,7 @@ struct AIDocumentSheet: View { /// populated without the host having to hold lists it does not otherwise need. @State private var lists: [OwnedList] = [] /// Documents the user owns, offered when deriving from an article. - var documents: [Document] = [] + var documents: [InterlinedDomain.Document] = [] /// Called after a drafted document is created, so the host can reload it. var onCreated: (() async -> Void)? diff --git a/App/Features/Documents/ConflictBannerView.swift b/App/Features/Documents/ConflictBannerView.swift index 04b1586..8997aeb 100644 --- a/App/Features/Documents/ConflictBannerView.swift +++ b/App/Features/Documents/ConflictBannerView.swift @@ -15,7 +15,7 @@ import InterlinedDomain struct ConflictBannerView: View { let pending: ConflictBannerViewModel.Pending - let onOpenLocalCopy: (Document.ID) -> Void + let onOpenLocalCopy: (InterlinedDomain.Document.ID) -> Void let onDismiss: () -> Void var body: some View { diff --git a/App/Features/Documents/DocumentEditorView.swift b/App/Features/Documents/DocumentEditorView.swift index d4d6070..9d67308 100644 --- a/App/Features/Documents/DocumentEditorView.swift +++ b/App/Features/Documents/DocumentEditorView.swift @@ -21,7 +21,7 @@ import Textual struct DocumentEditorView: View { let viewModel: DocumentEditorViewModel - let onOpenLocalCopy: (Document.ID) -> Void + let onOpenLocalCopy: (InterlinedDomain.Document.ID) -> Void var body: some View { VStack(spacing: 0) { diff --git a/App/Features/Documents/DocumentTemplatePickerView.swift b/App/Features/Documents/DocumentTemplatePickerView.swift index ad4b203..330a2da 100644 --- a/App/Features/Documents/DocumentTemplatePickerView.swift +++ b/App/Features/Documents/DocumentTemplatePickerView.swift @@ -36,7 +36,7 @@ struct DocumentTemplatePickerView: View { /// Called with the created document on success so the caller (the root /// view) can bind the editor to it. Not called on failure. - let onCreated: (Document) -> Void + let onCreated: (InterlinedDomain.Document) -> Void /// The built-in catalog to present. Defaults to the bundled built-ins; /// injectable so previews can substitute a list. diff --git a/App/Features/Documents/DocumentsListView.swift b/App/Features/Documents/DocumentsListView.swift index e0935c4..90436dd 100644 --- a/App/Features/Documents/DocumentsListView.swift +++ b/App/Features/Documents/DocumentsListView.swift @@ -9,10 +9,27 @@ import SwiftUI import InterlinedDomain +// `Document` is written as `InterlinedDomain.Document` throughout the SwiftUI +// files in this feature, and that qualification is load-bearing. +// +// The macOS 27 SDK added a `Document` **protocol** to SwiftUI +// (`protocol Document: ReadableDocument, WritableDocument`), which collides with +// the domain's `Document` **struct** in any file importing both — which is every +// documents view. Before Xcode 27 the bare name resolved; after it, the same +// source stopped compiling with `'Document' is ambiguous for type lookup` +// (GitHub #98). +// +// The domain type is not renamed: `Document` is the right name for it, it is +// correct across Kit, Domain, Persistence and their tests, and renaming a core +// model to dodge a collision in one consumer is the tail wagging the dog. A +// `typealias` would shorten the use sites at the cost of giving one concept two +// names. Only the SwiftUI-importing files need this; the view models import +// Foundation and Observation, not SwiftUI, and are unaffected. + struct DocumentsListView: View { let viewModel: DocumentsListViewModel - let onSelect: (Document.ID?) -> Void + let onSelect: (InterlinedDomain.Document.ID?) -> Void /// Source of the **Move to folder** destinations. Optional so the column /// still renders in isolation (previews, and any future host that has no @@ -21,7 +38,7 @@ struct DocumentsListView: View { /// Called with the document that was moved, so the host can rebind an open /// editor to the server's relocated copy. - var onMoved: ((Document) -> Void)? = nil + var onMoved: ((InterlinedDomain.Document) -> Void)? = nil var body: some View { List(selection: Binding( @@ -103,7 +120,7 @@ struct DocumentsListView: View { // MARK: - DocumentRowView private struct DocumentRowView: View { - let document: Document + let document: InterlinedDomain.Document var body: some View { VStack(alignment: .leading, spacing: 2) { diff --git a/App/Features/Documents/DocumentsRootView.swift b/App/Features/Documents/DocumentsRootView.swift index b52561b..dd1fa43 100644 --- a/App/Features/Documents/DocumentsRootView.swift +++ b/App/Features/Documents/DocumentsRootView.swift @@ -433,7 +433,7 @@ struct DocumentsRootView: View { /// was started from the editor, so there is exactly one optimistic-rollback /// implementation rather than two that can disagree. private func handleMove( - documentID: Document.ID, + documentID: InterlinedDomain.Document.ID, to destination: FolderNode.ID?, folderTree: FolderTreeViewModel, documentsList: DocumentsListViewModel, @@ -449,7 +449,7 @@ struct DocumentsRootView: View { } private func handleOpenLocalCopy( - _ id: Document.ID, + _ id: InterlinedDomain.Document.ID, documentsList: DocumentsListViewModel, editor: DocumentEditorViewModel ) { diff --git a/App/Features/Documents/PublicUserDocumentsView.swift b/App/Features/Documents/PublicUserDocumentsView.swift index 9613180..8914bef 100644 --- a/App/Features/Documents/PublicUserDocumentsView.swift +++ b/App/Features/Documents/PublicUserDocumentsView.swift @@ -111,7 +111,7 @@ struct PublicUserDocumentsView: View { private struct PublicDocumentRow: View { - let document: Document + let document: InterlinedDomain.Document var body: some View { VStack(alignment: .leading, spacing: 2) { diff --git a/App/Features/Search/SearchRootView.swift b/App/Features/Search/SearchRootView.swift index dad88a5..c5479de 100644 --- a/App/Features/Search/SearchRootView.swift +++ b/App/Features/Search/SearchRootView.swift @@ -266,7 +266,7 @@ struct SearchRootView: View { /// a relative "updated" stamp. Kept local to the Search feature because /// the Documents feature's own row component is file-private. private struct DocumentSearchRow: View { - let document: Document + let document: InterlinedDomain.Document var body: some View { VStack(alignment: .leading, spacing: 6) { diff --git a/Packages/InterlinedKit/Sources/InterlinedKit/APIClient/APIClient.swift b/Packages/InterlinedKit/Sources/InterlinedKit/APIClient/APIClient.swift index 053d4d3..baaafa2 100644 --- a/Packages/InterlinedKit/Sources/InterlinedKit/APIClient/APIClient.swift +++ b/Packages/InterlinedKit/Sources/InterlinedKit/APIClient/APIClient.swift @@ -28,6 +28,25 @@ public protocol APIClientProtocol: Sendable { /// Used for CSV export endpoints (`/api/exports/*`). func sendRaw(_ request: Request) async throws -> (Data, String?) + /// Executes a request and, on an HTTP failure, throws an ``APIFailure`` + /// **carrying the response body** instead of discarding it (GitHub #103). + /// + /// Opt-in on purpose. `APIError` keeps only the decoded `{error}` string, + /// which is all the UI needs for almost every failure — and not enough for + /// the few where the server answers a *question*: the list-schema + /// destructive-change guard names the columns that still hold data, and the + /// app-settings compare-and-set returns the current document on a conflict. + /// + /// Only callers that need the body use this. Everything else keeps throwing + /// plain `APIError`, so the 420 existing pattern-match sites are untouched. + /// + /// - Important: this throws `APIFailure`, **not** `APIError`. A caller that + /// opts in must catch accordingly; `APIFailure.underlyingError` carries the + /// `APIError` for anything that only wants the status or the message. + func sendCapturingFailure( + _ request: Request + ) async throws -> Response + /// Executes a request, decodes its JSON body into `Response`, and returns /// any rate-limit metadata extracted from the response headers. /// @@ -47,6 +66,29 @@ public protocol APIClientProtocol: Sendable { // MARK: - Default implementation extension APIClientProtocol { + + /// Default for conformers that cannot capture a response body — primarily + /// stubs and fakes. + /// + /// It wraps whatever `send(_:)` threw with `body: nil`, so an opted-in + /// caller still sees an `APIFailure` and its `details(as:)` simply answers + /// `nil`. That is the correct degradation: "no details available" is a real + /// state (a transport failure has no body either), so a stub that cannot + /// supply one is not lying. + /// + /// A non-`APIError` failure is rethrown untouched rather than wrapped — + /// a `CancellationError` is not an HTTP failure and must not start looking + /// like one. + public func sendCapturingFailure( + _ request: Request + ) async throws -> Response { + do { + return try await send(request) + } catch let error as APIError { + throw APIFailure(underlyingError: error, body: nil) + } + } + /// Conformers that do not need real rate-limit header extraction — primarily /// stubs and fakes — get this default which calls `send(_:)` and returns /// `nil`, correctly signalling "no limit enforced". @@ -106,7 +148,7 @@ public final class APIClient: APIClientProtocol { public func send( _ request: Request ) async throws -> Response { - let (data, _) = try await performWithSafetyNet(request) + let (data, _) = try await unwrappingFailure { try await performWithSafetyNet(request) } do { return try decoder.decode(Response.self, from: data) } catch { @@ -122,11 +164,48 @@ public final class APIClient: APIClientProtocol { } public func sendVoid(_ request: Request) async throws { - _ = try await performWithSafetyNet(request) + _ = try await unwrappingFailure { try await performWithSafetyNet(request) } + } + + public func sendCapturingFailure( + _ request: Request + ) async throws -> Response { + // Deliberately does NOT unwrap: this is the one entry point whose + // caller asked for the body. + let (data, _) = try await performWithSafetyNet(request) + do { + return try decoder.decode(Response.self, from: data) + } catch { + let detail = String(reflecting: error) + appLog.error("Decode failed [\(request.path)] type=\(String(describing: Response.self)): \(detail)") + // A *decode* failure is not an HTTP failure and has no server body + // to offer, so it surfaces as the plain `APIError` it has always + // been rather than an `APIFailure` with nothing in it. + throw APIError.decoding( + type: String(describing: Response.self), + message: detail + ) + } + } + + /// Runs `work` and flattens any `APIFailure` back to its `APIError`. + /// + /// The transport now raises the richer error so one code path serves both + /// entry points. Every caller that did not opt in must still see exactly + /// what it saw before — `catch let error as APIError` has to keep matching — + /// so the unwrap happens here rather than at 420 call sites. + private func unwrappingFailure( + _ work: () async throws -> T + ) async throws -> T { + do { + return try await work() + } catch let failure as APIFailure { + throw failure.underlyingError + } } public func sendRaw(_ request: Request) async throws -> (Data, String?) { - let (data, response) = try await performWithSafetyNet(request) + let (data, response) = try await unwrappingFailure { try await performWithSafetyNet(request) } let contentType = response.value(forHTTPHeaderField: "Content-Type") return (data, contentType) } @@ -134,7 +213,7 @@ public final class APIClient: APIClientProtocol { public func sendWithRateLimitInfo( _ request: Request ) async throws -> (Response, RateLimitInfo?) { - let (data, response) = try await performWithSafetyNet(request) + let (data, response) = try await unwrappingFailure { try await performWithSafetyNet(request) } do { let decoded = try decoder.decode(Response.self, from: data) // RateLimitInfo.parse returns nil when headers are absent — @@ -165,15 +244,22 @@ public final class APIClient: APIClientProtocol { ) async throws -> (Data, HTTPURLResponse) { do { return try await performWithRetry(request, forceSession: false) - } catch let error as APIError { + } catch let failure as APIFailure { // Safety net: a Bearer request that comes back 401 should // transparently try once via the session transport before we // give up. This catches future API drift in either direction. - if case .unauthorized = error, request.auth == .bearer { + // + // Matched on `underlyingError`, because the transport now raises + // `APIFailure` so the response body survives to a caller that asked + // for it (GitHub #103). The `APIFailure` is rethrown intact rather + // than flattened — flattening here would drop the body before + // `sendCapturingFailure` ever saw it, which is the entire point of + // the type. + if case .unauthorized = failure.underlyingError, request.auth == .bearer { appLog.warning("Bearer request returned 401 [\(request.path)] — retrying via session transport") return try await performWithRetry(request, forceSession: true) } - throw error + throw failure } } @@ -185,13 +271,16 @@ public final class APIClient: APIClientProtocol { while true { do { return try await performOnce(request, forceSession: forceSession) - } catch let error as APIError { - if let delay = retryPolicy.delay(error, attempt) { + } catch let failure as APIFailure { + // Same reasoning as the safety net above: the retry policy asks + // about the `APIError`, and the `APIFailure` is rethrown whole so + // the body reaches whoever asked for it. + if let delay = retryPolicy.delay(failure.underlyingError, attempt) { attempt += 1 try await Task.sleep(nanoseconds: UInt64(delay * 1_000_000_000)) continue } - throw error + throw failure } } } @@ -231,11 +320,17 @@ public final class APIClient: APIClientProtocol { guard (200..<300).contains(response.statusCode) else { let serverMessage = decodeServerMessage(from: data) appLog.notice("HTTP \(response.statusCode) [\(request.path)]: \(serverMessage ?? "no server message")") - throw APIError.from( + let apiError = APIError.from( statusCode: response.statusCode, serverMessage: serverMessage, retryAfter: parseRetryAfter(response.value(forHTTPHeaderField: "Retry-After")) ) + // The body is kept alongside the error so `sendCapturingFailure` + // can hand it to a caller that asked for it (GitHub #103). + // `send(_:)` unwraps this back to a plain `APIError`, so every + // existing caller is unaffected — the richer error never leaks into + // a code path that did not opt in. + throw APIFailure(underlyingError: apiError, body: data) } return (data, response) } diff --git a/Packages/InterlinedKit/Sources/InterlinedKit/Errors/APIFailure.swift b/Packages/InterlinedKit/Sources/InterlinedKit/Errors/APIFailure.swift new file mode 100644 index 0000000..64f9623 --- /dev/null +++ b/Packages/InterlinedKit/Sources/InterlinedKit/Errors/APIFailure.swift @@ -0,0 +1,74 @@ +import Foundation + +/// An HTTP failure **with its response body intact** (GitHub #103). +/// +/// `APIError` keeps only the decoded `{error}` string; everything else in the +/// body is discarded before any caller sees it. That is fine for the +/// overwhelming majority of failures, where a sentence is all the UI needs — and +/// wrong for the handful where the server answers a *question* rather than +/// reporting a malfunction: +/// +/// - `PUT /api/lists/{id}/schema` refuses a destructive rebuild with `400` plus +/// a **`propertiesWithData` array naming the columns** that still hold data. +/// The UI should name them and offer the confirmation; without the body it can +/// only repeat the server's sentence. +/// - The app-settings family is compare-and-set: a stale `baseVersion` answers +/// `409` with the **`current` document attached**, so a client can show what +/// changed, merge, or re-base and retry. +/// +/// ## Why this is a separate type rather than a case on `APIError` +/// +/// `APIError`'s cases carry a single `serverMessage` associated value, and +/// **420 sites across 78 files** pattern-match or construct them. Adding a +/// second associated value is a mechanical change to every one of those, +/// including every test — enormous churn, and every line of it a chance to get +/// a case wrong, in service of two call sites. +/// +/// So this is **opt-in**: `sendCapturingFailure(_:)` throws an `APIFailure`, and +/// only the callers that actually need the body use it. Every other call site +/// keeps throwing and catching plain `APIError`, unchanged. +/// +/// - Important: an `APIFailure` is **not** an `APIError`, so `catch let error as +/// APIError` will not match it. That is deliberate — a caller opts into the +/// richer error by choosing the richer send. `underlyingError` and the +/// forwarded `localizedDescription` mean nothing is lost if it reaches generic +/// error-display code. +public struct APIFailure: Error, Sendable { + + /// The failure as `APIError` models it. Callers that only care about the + /// status or the message switch on this exactly as they would have. + public let underlyingError: APIError + + /// The raw response body, when the failure carried one. + /// + /// `nil` for a transport failure, which never had a body to keep. + public let body: Data? + + public init(underlyingError: APIError, body: Data?) { + self.underlyingError = underlyingError + self.body = body + } + + /// Decodes the body as `T`, or `nil` when it was absent or did not match. + /// + /// Returning `nil` rather than throwing is the point: a caller asking for + /// details is asking an **optional** question, and a decode failure here + /// must degrade to today's behaviour — a message with no details — rather + /// than masking the original error with a decoding one. The failure the + /// caller is handling is the HTTP failure, not this. + public func details(as type: T.Type) -> T? { + guard let body else { return nil } + return try? JSONCoders.makeDecoder().decode(T.self, from: body) + } + + /// The HTTP status, when there was one. + public var httpStatusCode: Int? { underlyingError.httpStatusCode } +} + +extension APIFailure: LocalizedError, CustomStringConvertible { + /// Forwarded, so an `APIFailure` that reaches generic error-display code + /// reads exactly as the `APIError` would have. Nothing regresses by opting + /// in to the richer send. + public var errorDescription: String? { underlyingError.errorDescription } + public var description: String { underlyingError.description } +} diff --git a/Packages/InterlinedKit/Tests/InterlinedKitTests/APIFailureTests.swift b/Packages/InterlinedKit/Tests/InterlinedKitTests/APIFailureTests.swift new file mode 100644 index 0000000..df4e0d8 --- /dev/null +++ b/Packages/InterlinedKit/Tests/InterlinedKitTests/APIFailureTests.swift @@ -0,0 +1,132 @@ +// APIFailureTests +// +// The opt-in error carrier (GitHub #103). +// +// `APIError` keeps only the decoded `{error}` string, which is enough for almost +// every failure and not enough for the few where the server answers a *question* +// — the list-schema destructive-change guard names the columns that still hold +// data, and the app-settings compare-and-set returns the current document. +// +// The most important property under test is the one that is easy to break by +// accident: **opting in must not change anything for callers that did not**. + +import XCTest +@testable import InterlinedKit + +final class APIFailureTests: XCTestCase { + + private let baseURL = URL(string: "https://example.test")! + + private func makeClient() -> (APIClient, StubHTTPDataTransport) { + let transport = StubHTTPDataTransport() + let auth = DefaultAuthTransport( + tokenStore: InMemoryTokenStore(initial: "il_tok_test"), + sessionTransport: StubHTTPDataTransport(), + sessionEstablisher: NullSessionEstablisher() + ) + return (APIClient(baseURL: baseURL, transport: transport, authTransport: auth), transport) + } + + private struct Probe: Decodable, Sendable { let ok: Bool } + + /// The shape the list-schema guard actually answers with. + private struct SchemaConflict: Decodable { let propertiesWithData: [String]? } + + // MARK: - Happy path + + func test_givenAFailureWithABody_whenCapturing_thenTheDetailsDecode() async throws { + let (client, transport) = makeClient() + await transport.enqueue(.json(#"{"error":"Removing these columns would delete data.","code":"bad_request","propertiesWithData":["year","status"]}"#, status: 400)) + + do { + _ = try await client.sendCapturingFailure(Request(method: .get, path: "/probe", auth: .bearer)) + XCTFail("expected a failure") + } catch let failure as APIFailure { + XCTAssertEqual(failure.httpStatusCode, 400) + XCTAssertEqual( + failure.details(as: SchemaConflict.self)?.propertiesWithData, + ["year", "status"], + "the structured half of the body is what this type exists for" + ) + } + } + + func test_givenAFailure_whenCapturing_thenTheMessageIsUnchangedFromAPIError() async throws { + // Nothing regresses by opting in: an `APIFailure` that reaches generic + // error-display code has to read exactly as the `APIError` would have. + let (client, transport) = makeClient() + await transport.enqueue(.json(#"{"error":"title required"}"#, status: 400)) + + do { + _ = try await client.sendCapturingFailure(Request(method: .get, path: "/probe", auth: .bearer)) + XCTFail("expected a failure") + } catch let failure as APIFailure { + XCTAssertEqual(failure.underlyingError, .badRequest(serverMessage: "title required")) + XCTAssertEqual(failure.localizedDescription, APIError.badRequest(serverMessage: "title required").localizedDescription) + } + } + + // MARK: - The property that must not break + + func test_givenTheSameFailure_whenSentNormally_thenItIsStillAPlainAPIError() async throws { + // The transport raises `APIFailure` internally now so one code path + // serves both entry points. Every caller that did not opt in must still + // match `catch let error as APIError` — there are 420 such sites. + let (client, transport) = makeClient() + await transport.enqueue(.json(#"{"error":"title required","propertiesWithData":["year"]}"#, status: 400)) + + do { + _ = try await client.send(Request(method: .get, path: "/probe", auth: .bearer)) + XCTFail("expected a failure") + } catch let error as APIError { + XCTAssertEqual(error, .badRequest(serverMessage: "title required")) + } catch { + XCTFail("send(_:) must throw APIError, not \(type(of: error))") + } + } + + // MARK: - Invalid / absent details + + func test_givenABodyThatDoesNotMatch_whenDecodingDetails_thenItDegradesToNil() async throws { + // A caller asking for details is asking an *optional* question. A decode + // failure here must not mask the HTTP failure it is handling. + let (client, transport) = makeClient() + await transport.enqueue(.json(#"{"error":"nope"}"#, status: 400)) + + do { + _ = try await client.sendCapturingFailure(Request(method: .get, path: "/probe", auth: .bearer)) + XCTFail("expected a failure") + } catch let failure as APIFailure { + XCTAssertNil(failure.details(as: SchemaConflict.self)?.propertiesWithData) + XCTAssertEqual(failure.underlyingError, .badRequest(serverMessage: "nope")) + } + } + + func test_givenNoBodyAtAll_whenDecodingDetails_thenItIsNil() { + // A transport failure never had a body to keep. + let failure = APIFailure(underlyingError: .transport(message: "offline"), body: nil) + XCTAssertNil(failure.details(as: SchemaConflict.self)) + XCTAssertNil(failure.httpStatusCode) + } + + // MARK: - Boundary — a decode failure is not an HTTP failure + + func test_givenAnUndecodableSuccess_whenCapturing_thenItIsAPlainDecodingError() async throws { + // A 200 whose body does not match is a client-side decode problem with + // no server body to offer, so it surfaces as the `APIError.decoding` it + // has always been rather than an `APIFailure` carrying nothing useful. + let (client, transport) = makeClient() + await transport.enqueue(.json(#"{"unexpected":true}"#)) + + do { + _ = try await client.sendCapturingFailure(Request(method: .get, path: "/probe", auth: .bearer)) + XCTFail("expected a failure") + } catch let error as APIError { + guard case .decoding = error else { + return XCTFail("expected .decoding, got \(error)") + } + } catch { + XCTFail("expected APIError.decoding, got \(type(of: error))") + } + } +}