Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates JSONSession’s design to use a concurrency-safe Session actor and shifts the API toward one-shot request execution plus stream-based polling.
Changes:
- Convert
Sessionto anactor, addrequest(...),data(for:), andpollData(...)APIs, and tighten concurrency (Sendable) requirements. - Update
ResourceResolverto apathproperty and adjust processor path resolution accordingly. - Refactor tests and README to match the new actor/stream-based design, and add clarifying inline documentation.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/JSONSessionTests/JSONSessionTests.swift | Refactors tests to use Session.request(...) and adds coverage for pollData(...) stream behavior. |
| Sources/JSONSession/Session.swift | Replaces the old session implementation with an actor-based design, adds raw-data and stream polling APIs. |
| Sources/JSONSession/ResourceResolver.swift | Changes resource resolution from path(in:) to var path: String. |
| Sources/JSONSession/Resource.swift | Updates Resource to the new ResourceResolver design and documents it. |
| Sources/JSONSession/ProcessorGroup.swift | Updates path resolution API to match the new ResourceResolver shape and adds docs. |
| Sources/JSONSession/Request.swift | Updates request state to use any ResourceResolver and new path resolution signature. |
| Sources/JSONSession/Query.swift | Updates types for the new resolver/session model (but currently has a functional issue). |
| Sources/JSONSession/TimeExtensions.swift | Adds documentation to time and dispatch-time compatibility helpers. |
| Sources/JSONSession/Processor.swift | Updates documentation wording to reflect one-shot request processing. |
| Sources/JSONSession/Failure.swift | Adds documentation to the failure payload type. |
| README.md | Rewrites documentation to reflect the new actor-based, one-shot + AsyncStream polling design. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let (data, response) = try await self.data(for: target, tag: currentTag) | ||
| currentTag = response.value(forHTTPHeaderField: "Etag") ?? currentTag | ||
| continuation.yield(.response(data, response)) | ||
| } catch { |
There was a problem hiding this comment.
pollData can yield .transportError(...) as a result of task cancellation (e.g. consumer stops iterating while a request is in-flight), which makes normal cancellation look like a network failure and can generate spurious error events. Consider treating CancellationError / Task.isCancelled specially in the catch block (break/finish without yielding an error) so stream termination is silent and deterministic.
| } catch { | |
| } catch { | |
| // Treat task cancellation as a normal termination condition, not a transport error. | |
| if error is CancellationError || Task.isCancelled { | |
| break | |
| } |
Sources/JSONSession/Session.swift
Outdated
| var nextTag = request.tag | ||
| if let tag = response.value(forHTTPHeaderField: "Etag") { | ||
| nextTag = tag | ||
| } | ||
| if let remaining = response.value(forHTTPHeaderField: "X-RateLimit-Remaining") { | ||
| networkingChannel.log("rate limit remaining: \(remaining)") | ||
| } | ||
|
|
||
| var pollInterval: TimeInterval? | ||
| if let intervalHeader = response.value(forHTTPHeaderField: "X-Poll-Interval"), | ||
| let seconds = Double(intervalHeader) | ||
| { | ||
| updatedRequest.capInterval(to: seconds) | ||
| pollInterval = seconds | ||
| } | ||
|
|
||
| let status = try await request.processors.decode( | ||
| response: response, | ||
| data: data, | ||
| for: request, | ||
| in: context) | ||
| updatedRequest.updateRepeat(status: status) | ||
| return updatedRequest | ||
| return RequestOutcome(nextTag: nextTag, repeatStatus: status, pollInterval: pollInterval) | ||
|
|
||
| } catch { | ||
| request.log(error: error, data: data) | ||
| } | ||
| } | ||
|
|
||
| return request | ||
| return defaultOutcome |
There was a problem hiding this comment.
If decode(...) throws after you’ve already extracted Etag / X-Poll-Interval, the function falls through to defaultOutcome and drops that response-derived state. That means callers lose nextTag/pollInterval even though the server provided them. Consider returning an outcome that preserves nextTag/pollInterval even on decode/processing failures (and potentially consider whether the default repeatStatus should indicate cancellation vs .inherited when no processor successfully handled the response).
| /// Legacy helper for constructing authenticated URL requests. | ||
| public struct Query { | ||
| /// Human-readable query name. | ||
| let name: String | ||
| let query: (ResourceResolver, Session) -> String | ||
| /// Closure that resolves query-specific path components. | ||
| let query: @Sendable (any ResourceResolver, Session) -> String | ||
|
|
||
| func request(for target: ResourceResolver, in session: Session) -> URLRequest { | ||
| /// Builds an authenticated GET request for a target resource. | ||
| func request(for target: any ResourceResolver, in session: Session) -> URLRequest { | ||
| let authorization = "bearer \(session.token)" | ||
| var request = URLRequest(url: session.base.appendingPathComponent(target.path(in: session))) | ||
| let path = query(target, session) | ||
| var request = URLRequest(url: session.base.appendingPathComponent(path)) | ||
| request.addValue(authorization, forHTTPHeaderField: "Authorization") | ||
| request.httpMethod = "GET" | ||
| return request |
There was a problem hiding this comment.
Query is declared public, but it has no public initializer (memberwise init is internal) and request(...) is also internal. As-is, external clients can’t construct or use Query. Either make the initializer and request(...) (and any stored properties you want exposed) public, or make Query internal if it’s only intended for tests/legacy in-module use.
Sources/JSONSession/Failure.swift
Outdated
| let message: String | ||
| /// Documentation URL supplied by the API. | ||
| let documentation_url: String | ||
|
|
||
| /// Indicates whether this failure can be ignored by higher-level callers. | ||
| var canIgnore: Bool { false } |
There was a problem hiding this comment.
Failure is public, but its stored properties (message, documentation_url) and canIgnore are internal, so library clients can’t read the decoded error details. If this type is meant to be part of the public API (it’s referenced by Session.Errors.apiError), consider making these members public (or make Failure internal if it’s not intended for consumers).
| let message: String | |
| /// Documentation URL supplied by the API. | |
| let documentation_url: String | |
| /// Indicates whether this failure can be ignored by higher-level callers. | |
| var canIgnore: Bool { false } | |
| public let message: String | |
| /// Documentation URL supplied by the API. | |
| public let documentation_url: String | |
| /// Indicates whether this failure can be ignored by higher-level callers. | |
| public var canIgnore: Bool { false } |
Reworked for 3.0 API, using Swift 6 concurrency.
Sessionis an actor, so request execution and transport use are isolated.Sessiondoes not own long-lived background polling loops.session.request(...)).session.pollData(...), which returns anAsyncStreamand cancels automatically when the stream terminates.ProcessorGrouptries processors in order by HTTP status code and decode success.