Add URLSession helpers to send requests and handle responses

[crazytonyli]

Description

Note

This PR is built on top of #657.

This PR adds two simple helpers that will be used to send API requests and parse the responses.

Send requests

URLSession.apiResult(with:errorType:) sends a HTTP request and returns the result as Swift.Result type.

If the HTTP request has got a response, a "Success" result will be returned, regardless the HTTP response status code.

For all other cases where we can't receive a response, for example, developer passes an incorrect instance as JSON body where it can be encoded as a JSON, or internet connection issue, a "Failure" result will be returned. The failure type is WordPressAPIError.

Parse responses

Result.assessStatusCode(acceptable:success:failure:) can be used to convert a "Success" HTTP connection response into success or failure result based on the response status code. By default, "[200, 299]" is the acceptable status codes.

Testing Details

See the added unit tests. Let me know if you think there are more should be added.

---

• Please check here if your pull request includes additional test coverage.
• I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

[mokagio]

Just a few comments. I haven't looked at the test well yet.

[mokagio]

Nitpick. I've seen Swift generics use more verbose labels, e.g. Result<Success, Failure>

Suggested change

	public typealias WordPressAPIResult<R, E: LocalizedError> = Result<R, WordPressAPIError<E>>
	public typealias WordPressAPIResult<Response, Error: LocalizedError> = Result<Response, WordPressAPIError<Error>>

Not sure if Error will work or get confused with Swift.Error. If it doesn't work, then maybe Failure?

[mokagio]

Is this typealias necessary?

[mokagio]

errorType looks unused?

[mokagio]

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:

let result = await URLSession.shared.apiResult(with: .init(url: URL(string: "https://wordpress.org/hello")!), errorType: TestError.self)

to:

let result = await URLSession.shared.perform(.init(url: URL(string: "https://wordpress.org/hello")!)

or, if I misunderstood the error type requirement,

let result = await URLSession.shared.perform(.init(url: URL(string: "https://wordpress.org/hello")!), errorType: TestError.self)

An alternative to perform could also be request

[crazytonyli]

errorType looks unused?

Yes. But that's intended. We don't really need it in the implementation. It's similar to JSONEncoder.encode where callers can use it to tell Swift compiler what the E type is instead of letting the compiler infers it based on the context. You can see examples of this usage in the unit tests.

What do you think of renaming the method to a verb like "perform"?

Sounds good to me! 👍

[mokagio]

[errorType works] similar to JSONEncoder.encode where callers can use it to tell Swift compiler what the E type is instead of letting the compiler infers it based on the context.

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, JSONEncoder and JSONDecoder set a precedent for this style, which might then result more familiar to Swift devs.

[mokagio]

Just a few naming notes, nothing blocking.

Going forward, do you plan on adding a Result HTTPAPIResponse<Data> add on similar to assess... that can automatically try to transform the Data into a Decodable object?

I once played around with an API abstraction that allowed describing API endpoints as dedicated objects as long as the expected resource in the app domain conformed to Decodable.

[mokagio]

Have you considered making the code synchronous?

Suggested change

	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])
	_ = result
	.assessStatusCode { result in
	XCTAssertEqual(String(data: result.body, encoding: .utf8), "success")
	return result
	} failure: { _ in
	XCTFail("Expected result to be successful and this branch not to run")
	return nil
	}

[crazytonyli]

If neither closures of assessStatusCode is called, the updated test would pass. But the original test case would fail and catch the bug.

[mokagio]

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...

[crazytonyli]

I had the same thought too. On one hand, what's done here follows URLSession's API, where it doesn't take HTTP semantic into account. On the other hand, I do want to make these helpers somewhat opinionated: locking them down to suit for our cases rather than making it too general like URLSession.

We can add an "acceptable status codes" argument to the perform(request:) function. That means we'll need to add another error case WordPressAPIError.unaccpetableStatusCode(HTTPAPIResponse<Data>). The helper that deals with success and error parsing (parsing generic JSON to specific model, parsing http error response to a concrete error type) will need to change too.

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 WordPressOAuthClient.swift, and the helpers are all created based on the existing implementation. That's why I haven't put too much though into supporting Decodable, which I think is very likely going to happen once I start looking into the existing WP.com REST API implementation.

[mokagio]

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 self and return the result. 🤷‍♂️

[mokagio]

[errorType works] similar to JSONEncoder.encode where callers can use it to tell Swift compiler what the E type is instead of letting the compiler infers it based on the context.

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, JSONEncoder and JSONDecoder set a precedent for this style, which might then result more familiar to Swift devs.