Add URLSession helpers to send requests and handle responses #658
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| import Foundation | ||
|
|
||
| public typealias WordPressAPIResult<Response, Error: LocalizedError> = Result<Response, WordPressAPIError<Error>> | ||
|
|
||
| struct HTTPAPIResponse<Body> { | ||
| var response: HTTPURLResponse | ||
| var body: Body | ||
| } | ||
|
|
||
| extension URLSession { | ||
|
|
||
| func perform<E: LocalizedError>( | ||
| request builder: HTTPRequestBuilder, | ||
| errorType: E.Type = E.self | ||
| ) async -> WordPressAPIResult<HTTPAPIResponse<Data>, E> { | ||
| 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)) | ||
| } | ||
| } | ||
|
|
||
| let (body, response) = result | ||
|
|
||
| guard let response = response as? HTTPURLResponse else { | ||
| return .failure(.unparsableResponse(response: nil, body: body)) | ||
| } | ||
|
|
||
| return .success(.init(response: response, body: body)) | ||
| } | ||
|
|
||
| } | ||
|
|
||
| extension Result where Success == HTTPAPIResponse<Data> { | ||
|
|
||
| func assessStatusCode<S, E: LocalizedError>( | ||
| acceptable: [ClosedRange<Int>] = [200...299], | ||
| success: (Success) -> S?, | ||
| failure: (Success) -> E? | ||
| ) -> WordPressAPIResult<S, E> where Failure == WordPressAPIError<E> { | ||
| flatMap { response in | ||
| if acceptable.contains(where: { $0 ~= response.response.statusCode }) { | ||
| if let result = success(response) { | ||
| return .success(result) | ||
| } else { | ||
| return .failure(.unparsableResponse(response: response.response, body: response.body)) | ||
| } | ||
| } else { | ||
| if let endpointError = failure(response) { | ||
| return .failure(.endpointError(endpointError)) | ||
| } else { | ||
| return .failure(.unparsableResponse(response: response.response, body: response.body)) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,130 @@ | ||||||||||||||||||||||||||||||||||||||||
| import Foundation | ||||||||||||||||||||||||||||||||||||||||
| import XCTest | ||||||||||||||||||||||||||||||||||||||||
| import OHHTTPStubs | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| @testable import WordPressKit | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| class URLSessionHelperTests: XCTestCase { | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| override func tearDown() { | ||||||||||||||||||||||||||||||||||||||||
| super.tearDown() | ||||||||||||||||||||||||||||||||||||||||
| HTTPStubs.removeAllStubs() | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| func testConnectionError() async throws { | ||||||||||||||||||||||||||||||||||||||||
| stub(condition: isPath("/hello")) { _ in | ||||||||||||||||||||||||||||||||||||||||
| HTTPStubsResponse(error: URLError(.serverCertificateUntrusted)) | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| let result = await URLSession.shared.perform(request: .init(url: URL(string: "https://wordpress.org/hello")!), errorType: TestError.self) | ||||||||||||||||||||||||||||||||||||||||
| do { | ||||||||||||||||||||||||||||||||||||||||
| _ = try result.get() | ||||||||||||||||||||||||||||||||||||||||
| XCTFail("The above call should throw") | ||||||||||||||||||||||||||||||||||||||||
| } catch let WordPressAPIError<TestError>.connection(error) { | ||||||||||||||||||||||||||||||||||||||||
| XCTAssertEqual(error.code, URLError.Code.serverCertificateUntrusted) | ||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||
| XCTFail("Unknown error: \(error)") | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| func test200() async throws { | ||||||||||||||||||||||||||||||||||||||||
| stub(condition: isPath("/hello")) { _ in | ||||||||||||||||||||||||||||||||||||||||
| HTTPStubsResponse(data: "success".data(using: .utf8)!, statusCode: 200, headers: nil) | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| let result = await URLSession.shared.perform(request: .init(url: URL(string: "https://wordpress.org/hello")!), errorType: TestError.self) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // The result is a successful result. This line should not throw | ||||||||||||||||||||||||||||||||||||||||
| _ = try result.get() | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| let expectation = expectation(description: "API call returns a successful result") | ||||||||||||||||||||||||||||||||||||||||
| _ = result | ||||||||||||||||||||||||||||||||||||||||
| .assessStatusCode { result in | ||||||||||||||||||||||||||||||||||||||||
| XCTAssertEqual(String(data: result.body, encoding: .utf8), "success") | ||||||||||||||||||||||||||||||||||||||||
| expectation.fulfill() | ||||||||||||||||||||||||||||||||||||||||
| return result | ||||||||||||||||||||||||||||||||||||||||
| } failure: { _ in | ||||||||||||||||||||||||||||||||||||||||
| // Do nothing | ||||||||||||||||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| await fulfillment(of: [expectation]) | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+40
to
+50
Contributor
There was a problem hiding this comment. Have you considered making the code synchronous?
Suggested change
Contributor
Author
There was a problem hiding this comment. If neither closures of |
||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| func testUnacceptable500() async throws { | ||||||||||||||||||||||||||||||||||||||||
| stub(condition: isPath("/hello")) { _ in | ||||||||||||||||||||||||||||||||||||||||
| HTTPStubsResponse(data: "Internal server error".data(using: .utf8)!, statusCode: 500, headers: nil) | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| let result = await URLSession.shared.perform(request: .init(url: URL(string: "https://wordpress.org/hello")!), errorType: TestError.self) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // The result is a successful result. This line should not throw | ||||||||||||||||||||||||||||||||||||||||
| _ = try result.get() | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+58
to
+61
Contributor
There was a problem hiding this comment. I was surprised for a moment. "The result is a successful result," but the stub was configured to "fail" the request. The difference, however, is that the request succeeded to get a response and that the response contained a failure. I wonder if there's a way to make this clearer from consumers. Or maybe I'm overthinking it...
Contributor
Author
There was a problem hiding this comment. I had the same thought too. On one hand, what's done here follows We can add an "acceptable status codes" argument to the I think these changes may potentially be implemented later on. These helpers are internal API after they, they can be easily changed without much affects to others. At the moment, I only looked into the code in |
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| let expectation = expectation(description: "API call returns server error") | ||||||||||||||||||||||||||||||||||||||||
| _ = result | ||||||||||||||||||||||||||||||||||||||||
| .assessStatusCode { result in | ||||||||||||||||||||||||||||||||||||||||
| return result | ||||||||||||||||||||||||||||||||||||||||
| } failure: { result in | ||||||||||||||||||||||||||||||||||||||||
| XCTAssertEqual(String(data: result.body, encoding: .utf8), "Internal server error") | ||||||||||||||||||||||||||||||||||||||||
| expectation.fulfill() | ||||||||||||||||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| await fulfillment(of: [expectation]) | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| func testAcceptable404() async throws { | ||||||||||||||||||||||||||||||||||||||||
| stub(condition: isPath("/hello")) { _ in | ||||||||||||||||||||||||||||||||||||||||
| HTTPStubsResponse(data: "Not found".data(using: .utf8)!, statusCode: 404, headers: nil) | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| let result = await URLSession.shared.perform(request: .init(url: URL(string: "https://wordpress.org/hello")!), errorType: TestError.self) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // The result is a successful result. This line should not throw | ||||||||||||||||||||||||||||||||||||||||
| _ = try result.get() | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| let expectation = expectation(description: "API call returns not found") | ||||||||||||||||||||||||||||||||||||||||
| _ = result | ||||||||||||||||||||||||||||||||||||||||
| .assessStatusCode(acceptable: [200...299, 400...499]) { result in | ||||||||||||||||||||||||||||||||||||||||
| XCTAssertEqual(String(data: result.body, encoding: .utf8), "Not found") | ||||||||||||||||||||||||||||||||||||||||
| expectation.fulfill() | ||||||||||||||||||||||||||||||||||||||||
| return result | ||||||||||||||||||||||||||||||||||||||||
| } failure: { result in | ||||||||||||||||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| await fulfillment(of: [expectation]) | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| func testParseError() async throws { | ||||||||||||||||||||||||||||||||||||||||
| stub(condition: isPath("/hello")) { _ in | ||||||||||||||||||||||||||||||||||||||||
| HTTPStubsResponse(data: "Not found".data(using: .utf8)!, statusCode: 404, headers: nil) | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| let result = await URLSession.shared.perform(request: .init(url: URL(string: "https://wordpress.org/hello")!), errorType: TestError.self) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // The result is a successful result. This line should not throw | ||||||||||||||||||||||||||||||||||||||||
| _ = try result.get() | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| let expectation = expectation(description: "API call returns not found") | ||||||||||||||||||||||||||||||||||||||||
| let parsedResult = result | ||||||||||||||||||||||||||||||||||||||||
| .assessStatusCode { result in | ||||||||||||||||||||||||||||||||||||||||
| return result | ||||||||||||||||||||||||||||||||||||||||
| } failure: { result in | ||||||||||||||||||||||||||||||||||||||||
| expectation.fulfill() | ||||||||||||||||||||||||||||||||||||||||
| if result.response.statusCode == 404 { | ||||||||||||||||||||||||||||||||||||||||
| return .postNotFound | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| await fulfillment(of: [expectation]) | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+104
to
+118
Contributor
There was a problem hiding this comment. If I read this correctly based also on the info in the PR description, these tests are previews of how we'd use the code in real life to make API requests. With that in mind, what do you think of using a name like map? let session = ...
let result = await session.perform(request: .init(url: URL(string: "https://wordpress.org/hello")!), errorType: TestError.self)
.map(
acceptableStatusCodes: [200...299],
success: { result in ... }
failure: { result in ... }
)With default parameter: let session = ...
let result = await session.perform(request: .init(url: URL(string: "https://wordpress.org/hello")!), errorType: TestError.self)
.map(
success: { result in ... }
failure: { result in ... }
)I'm suggesting this because "assess" doesn't make me think of an operation that would apply the given transformations to |
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| if case .failure(WordPressAPIError<TestError>.endpointError(.postNotFound)) = parsedResult { | ||||||||||||||||||||||||||||||||||||||||
| // DO nothing | ||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||
| XCTFail("Unexpected result: \(parsedResult)") | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| private enum TestError: LocalizedError { | ||||||||||||||||||||||||||||||||||||||||
| case postNotFound | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errorTypelooks unused?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll keep rolling with the unused assumption and make an additional suggestion, which would be valid anyway but results in my opinion cleaner this way.
What do you think of renaming the method to a verb like "perform"? The fact that the method returns an API result is already encoded in the type signature.
This way, including the removed error type, we could go from a usage like:
to:
or, if I misunderstood the error type requirement,
An alternative to
performcould also berequestThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. But that's intended. We don't really need it in the implementation. It's similar to
JSONEncoder.encodewhere callers can use it to tell Swift compiler what theEtype is instead of letting the compiler infers it based on the context. You can see examples of this usage in the unit tests.Sounds good to me! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. If we don't specify it, then we're left to explicitly set the type of the result we expect from the API call, as can be seen in this edit I made locally.
To be honest, I'd rather to that than pass unused arguments, but I can see how the current version is easier on the eyes. Also, as you point out,
JSONEncoderandJSONDecoderset a precedent for this style, which might then result more familiar to Swift devs.