Skip to content
This repository was archived by the owner on Sep 15, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 70 additions & 15 deletions WordPressKit/HTTPClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -9,37 +9,92 @@ struct HTTPAPIResponse<Body> {

extension URLSession {

/// Send a HTTP request and return its response as a `WordPressAPIResult` instance.
///
/// ## Progress Tracking and Cancellation
///
/// You can track the HTTP request's overall progress by passing a `Progress` instance to the `fulfillingProgress`
/// parameter, which must satisify following requirements:
/// - `totalUnitCount` must not be zero.
/// - `completedUnitCount` must be zero.
/// - It's used exclusivity for tracking the HTTP request overal progress: No children in its progress tree.

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.

Suggested change
/// - It's used exclusivity for tracking the HTTP request overal progress: No children in its progress tree.
/// - It's used exclusively for tracking the HTTP request overall progress: No children in its progress tree.

/// - `cancellationHandler` must be nil. You can call `fulfillingProgress.cancel()` to cancel the ongoing HTTP request.
Comment on lines +16 to +21

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.

What do you think about encoding these constraints in either a dedicated type that wraps Progress or a subclass/pre-configured instance?

Type wrapping Progress

More coding and less flexibility for advanced users, but no chance of misuse since we'd be controlling everything from here

Subclass / Preconfigured instance

Could be misuses, but by letting the method accept a Progress type, we'd give power users more flexibility

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.

Update: I saw the assert below. They do a good job at ensuring runtime safety. Still, from a "how many things does a user need to know?" perspective, I think leveraging the type system somehow would be beneficial.

///
/// Upon completion, the HTTP request's progress fulfills the `fulfillingProgress`.
///
/// - Parameters:
/// - builder: A `HTTPRequestBuilder` instance that represents an HTTP request to be sent.
/// - acceptableStatusCodes: HTTP status code ranges that are considered a successful response. Responses with
/// a status code outside of these ranges are returned as a `WordPressAPIResult.unacceptableStatusCode` instance.
/// - parentProgress: A `Progress` instance that will be used as the parent progress of the HTTP request's overall
/// progress. See the function documentation regarding requirements on this argument.
/// - errorType: The concret endpoint error type.
func perform<E: LocalizedError>(
request builder: HTTPRequestBuilder,
acceptableStatusCodes: [ClosedRange<Int>] = [200...299],
fulfillingProgress parentProgress: Progress? = nil,
errorType: E.Type = E.self
) async -> WordPressAPIResult<HTTPAPIResponse<Data>, E> {
if let parentProgress {
assert(parentProgress.completedUnitCount == 0 && parentProgress.totalUnitCount > 0, "Invalid parent progress")
assert(parentProgress.cancellationHandler == nil, "The progress instance's cancellationHandler property must be nil")
}

guard let request = try? builder.build() else {
return .failure(.requestEncodingFailure)
}

let result: (Data, URLResponse)
do {
result = try await data(for: request)
} catch {
if let urlError = error as? URLError {
return .failure(.connection(urlError))
} else {
return .failure(.unknown(underlyingError: error))
return await withCheckedContinuation { continuation in
let task = dataTask(with: request) { data, response, error in
let result: WordPressAPIResult<HTTPAPIResponse<Data>, E> = Self.parseResponse(
data: data,
response: response,
error: error,
acceptableStatusCodes: acceptableStatusCodes
)

continuation.resume(returning: result)
}
}
task.resume()

let (body, response) = result
if let parentProgress, parentProgress.totalUnitCount > parentProgress.completedUnitCount {
let pending = parentProgress.totalUnitCount - parentProgress.completedUnitCount

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.

Do we need this math given the assert above for completedUnitCount == 0?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, I don't think it's needed. But, in my head, the code expresses the idea more explicitly: letting the task.progress fill parentProgress without overflowing.

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.

Fair 👍

parentProgress.addChild(task.progress, withPendingUnitCount: pending)

guard let response = response as? HTTPURLResponse else {
return .failure(.unparsableResponse(response: nil, body: body))
parentProgress.cancellationHandler = { [weak task] in
task?.cancel()
}
}
}
}

guard acceptableStatusCodes.contains(where: { $0 ~= response.statusCode }) else {
return .failure(.unacceptableStatusCode(response: response, body: body))
private static func parseResponse<E: LocalizedError>(
data: Data?,
response: URLResponse?,
error: Error?,
acceptableStatusCodes: [ClosedRange<Int>]
) -> WordPressAPIResult<HTTPAPIResponse<Data>, E> {
let result: WordPressAPIResult<HTTPAPIResponse<Data>, E>

if let error {
if let urlError = error as? URLError {
result = .failure(.connection(urlError))
} else {
result = .failure(.unknown(underlyingError: error))
}
} else {
if let httpResponse = response as? HTTPURLResponse {
if acceptableStatusCodes.contains(where: { $0 ~= httpResponse.statusCode }) {
result = .success(HTTPAPIResponse(response: httpResponse, body: data ?? Data()))
} else {
result = .failure(.unacceptableStatusCode(response: httpResponse, body: data ?? Data()))
}
} else {
result = .failure(.unparsableResponse(response: nil, body: data))
}
}

return .success(.init(response: response, body: body))
return result
}

}
Expand Down
37 changes: 37 additions & 0 deletions WordPressKitTests/Utilities/URLSessionHelperTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,43 @@ class URLSessionHelperTests: XCTestCase {

try XCTAssertEqual(result.get().title, "Hello Post")
}

func testProgressTracking() async throws {
stub(condition: isPath("/hello")) { _ in
HTTPStubsResponse(data: "success".data(using: .utf8)!, statusCode: 200, headers: nil)
}

let progress = Progress.discreteProgress(totalUnitCount: 20)
XCTAssertEqual(progress.completedUnitCount, 0)
XCTAssertEqual(progress.fractionCompleted, 0)

let _ = await URLSession.shared.perform(request: .init(url: URL(string: "https://wordpress.org/hello")!), fulfillingProgress: progress, errorType: TestError.self)
XCTAssertEqual(progress.completedUnitCount, 20)
XCTAssertEqual(progress.fractionCompleted, 1)
}

func testCancellation() async throws {
// Give a slow HTTP request that takes 0.5 second to complete
stub(condition: isPath("/hello")) { _ in
let response = HTTPStubsResponse(data: "success".data(using: .utf8)!, statusCode: 200, headers: nil)
response.responseTime = 0.5
return response
}

// and cancelling it (in 0.1 second) before it completes
let progress = Progress.discreteProgress(totalUnitCount: 20)
DispatchQueue.main.asyncAfter(deadline: .now() + .milliseconds(100)) {
progress.cancel()
}

// The result should be an cancellation result
let result = await URLSession.shared.perform(request: .init(url: URL(string: "https://wordpress.org/hello")!), fulfillingProgress: progress, errorType: TestError.self)
if case let .failure(.connection(urlError)) = result, urlError.code == .cancelled {
// Do nothing
} else {
XCTFail("Unexpected result: \(result)")
}
}
}

private enum TestError: LocalizedError, Equatable {
Expand Down