Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use URLSession to send XMLRPC requests #719

Merged
merged 9 commits into from
Feb 6, 2024
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ _None._

### Internal Changes

_None._
- When enabled, `WrodPressOrgXMLRPCApi` sends HTTP requests using URLSession instead of Alamofire. [#719]

## 13.0.0

Expand Down
188 changes: 177 additions & 11 deletions WordPressKit/WordPressOrgXMLRPCApi.swift
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,20 @@ open class WordPressOrgXMLRPCApi: NSObject {
parameters: [AnyObject]?,
success: @escaping SuccessResponseBlock,
failure: @escaping FailureReponseBlock) -> Progress? {
guard !WordPressOrgXMLRPCApi.useURLSession else {
let progress = Progress.discreteProgress(totalUnitCount: 100)
Task { @MainActor in
let result = await self.call(method: method, parameters: parameters, fulfilling: progress, streaming: false)
switch result {
case let .success(response):
success(response.body, response.response)
case let .failure(error):
failure(error.asNSError(), error.response)
}
}
return progress
}

// Encode request
let request: URLRequest
do {
Expand Down Expand Up @@ -214,6 +228,20 @@ open class WordPressOrgXMLRPCApi: NSObject {
parameters: [AnyObject]?,
success: @escaping SuccessResponseBlock,
failure: @escaping FailureReponseBlock) -> Progress? {
guard !WordPressOrgXMLRPCApi.useURLSession else {
let progress = Progress.discreteProgress(totalUnitCount: 100)
Task { @MainActor in
let result = await self.call(method: method, parameters: parameters, fulfilling: progress, streaming: true)
switch result {
case let .success(response):
success(response.body, response.response)
case let .failure(error):
failure(error.asNSError(), error.response)
}
}
return progress
}

let progress: Progress = Progress.discreteProgress(totalUnitCount: 1)
progress.isCancellable = true
DispatchQueue.global().async {
Expand Down Expand Up @@ -254,6 +282,33 @@ open class WordPressOrgXMLRPCApi: NSObject {
return progress
}

/// Call an XMLRPC method.
///
/// ## Error handling
///
/// Unlike the closure-based APIs, this method returns a concrete error type. You should consider handling the errors
/// as they are, instead of casting them to `NSError` instance. But in case you do need to cast them to `NSError`,
/// considering using the `asNSError` function if you need backward compatibility with existing code.
///
/// - Parameters:
/// - streaming: set to `true` if there are large data (i.e. uploading files) in given `parameters`. `false` by default.
/// - Returns: A `Result` type that contains the XMLRPC success or failure result.
func call(method: String, parameters: [AnyObject]?, fulfilling progress: Progress, streaming: Bool = false) async -> WordPressAPIResult<HTTPAPIResponse<AnyObject>, WordPressOrgXMLRPCApiFault> {
let session = streaming ? uploadURLSession : urlSession
let builder = HTTPRequestBuilder(url: endpoint)
.method(.post)
.body(xmlrpc: method, parameters: parameters)
return await session
.perform(
request: builder,
// All HTTP responses are treated as successful result. Error handling will be done in `decodeXMLRPCResult`.
acceptableStatusCodes: [1...999],
fulfilling: progress,
errorType: WordPressOrgXMLRPCApiFault.self
)
.decodeXMLRPCResult()
}

// MARK: - Request Building

private func requestWithMethod(_ method: String, parameters: [AnyObject]?) throws -> URLRequest {
Expand Down Expand Up @@ -294,9 +349,9 @@ open class WordPressOrgXMLRPCApi: NSObject {
let httpResponse = urlResponse as? HTTPURLResponse,
let contentType = httpResponse.allHeaderFields["Content-Type"] as? String, error == nil else {
if let unwrappedError = error {
throw convertError(unwrappedError, data: originalData)
throw Self.convertError(unwrappedError, data: originalData)
} else {
throw convertError(WordPressOrgXMLRPCApiError.unknown as NSError, data: originalData)
throw Self.convertError(WordPressOrgXMLRPCApiError.unknown as NSError, data: originalData)
}
}

Expand All @@ -306,20 +361,20 @@ open class WordPressOrgXMLRPCApi: NSObject {
// it will return a valid fault payload with a non-200
throw decoderError
} else {
throw convertError(WordPressOrgXMLRPCApiError.httpErrorStatusCode as NSError, data: originalData, statusCode: httpResponse.statusCode)
throw Self.convertError(WordPressOrgXMLRPCApiError.httpErrorStatusCode as NSError, data: originalData, statusCode: httpResponse.statusCode)
}
}

if ["application/xml", "text/xml"].filter({ (type) -> Bool in return contentType.hasPrefix(type)}).count == 0 {
throw convertError(WordPressOrgXMLRPCApiError.responseSerializationFailed as NSError, data: originalData)
throw Self.convertError(WordPressOrgXMLRPCApiError.responseSerializationFailed as NSError, data: originalData)
}

guard let decoder = WPXMLRPCDecoder(data: data) else {
throw WordPressOrgXMLRPCApiError.responseSerializationFailed
}
guard !(decoder.isFault()), let responseXML = decoder.object() else {
if let decoderError = decoder.error() {
throw convertError(decoderError as NSError, data: data)
throw Self.convertError(decoderError as NSError, data: data)
} else {
throw WordPressOrgXMLRPCApiError.responseSerializationFailed
}
Expand All @@ -332,13 +387,13 @@ open class WordPressOrgXMLRPCApi: NSObject {
@objc public static let WordPressOrgXMLRPCApiErrorKeyDataString: NSError.UserInfoKey = "WordPressOrgXMLRPCApiErrorKeyDataString"
@objc public static let WordPressOrgXMLRPCApiErrorKeyStatusCode: NSError.UserInfoKey = "WordPressOrgXMLRPCApiErrorKeyStatusCode"

private func convertError(_ error: NSError, data: Data?, statusCode: Int? = nil) -> NSError {
fileprivate static func convertError(_ error: NSError, data: Data?, statusCode: Int? = nil) -> NSError {
let responseCode = statusCode == 403 ? 403 : error.code
if let data = data {
var userInfo: [AnyHashable: Any] = error.userInfo
userInfo[type(of: self).WordPressOrgXMLRPCApiErrorKeyData] = data
userInfo[type(of: self).WordPressOrgXMLRPCApiErrorKeyDataString] = NSString(data: data, encoding: String.Encoding.utf8.rawValue)
userInfo[type(of: self).WordPressOrgXMLRPCApiErrorKeyStatusCode] = statusCode
var userInfo: [String: Any] = error.userInfo
userInfo[Self.WordPressOrgXMLRPCApiErrorKeyData as String] = data
userInfo[Self.WordPressOrgXMLRPCApiErrorKeyDataString as String] = NSString(data: data, encoding: String.Encoding.utf8.rawValue)
userInfo[Self.WordPressOrgXMLRPCApiErrorKeyStatusCode as String] = statusCode
Comment on lines +394 to +396
Copy link
Contributor

Choose a reason for hiding this comment

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

I was surprised by the as String so I drilled into the type definition. All those keys are NSError.UserInfoKey which is an NSString.

I wonder why Apple decided to define userInfo keys as NSString when other similar entities are Strings:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Side note: From the quick look I had at the code, I got the impression all usages ended up cast as String.

Unless I'm wrong, we could followup with a simplification PR that defines those keys as String.

userInfo[NSLocalizedFailureErrorKey] = error.localizedDescription

if let statusCode = statusCode, (400..<600).contains(statusCode) {
Expand All @@ -348,7 +403,7 @@ open class WordPressOrgXMLRPCApi: NSObject {
userInfo[NSLocalizedFailureReasonErrorKey] = error.localizedFailureReason
}

return NSError(domain: error.domain, code: responseCode, userInfo: userInfo as? [String: Any])
return NSError(domain: error.domain, code: responseCode, userInfo: userInfo)
}
return error
}
Expand Down Expand Up @@ -451,3 +506,114 @@ extension WordPressOrgXMLRPCApiError: LocalizedError {
}
}
}

public struct WordPressOrgXMLRPCApiFault: LocalizedError, HTTPURLResponseProviding {
public var response: HTTPAPIResponse<Data>
public let code: Int?
public let message: String?

public init(response: HTTPAPIResponse<Data>, code: Int?, message: String?) {
self.response = response
self.code = code
self.message = message
}

public var errorDescription: String? {
message
}

public var httpResponse: HTTPURLResponse? {
response.response
}
}

private extension WordPressAPIResult<HTTPAPIResponse<Data>, WordPressOrgXMLRPCApiFault> {

func decodeXMLRPCResult() -> WordPressAPIResult<HTTPAPIResponse<AnyObject>, WordPressOrgXMLRPCApiFault> {
// This is a re-implementation of `WordPressOrgXMLRPCApi.handleResponseWithData` function:
// https://github.com/wordpress-mobile/WordPressKit-iOS/blob/11.0.0/WordPressKit/WordPressOrgXMLRPCApi.swift#L265
flatMap { response in
guard let contentType = response.response.allHeaderFields["Content-Type"] as? String else {
return .failure(.unparsableResponse(response: response.response, body: response.body, underlyingError: WordPressOrgXMLRPCApiError.unknown))
}

if (400..<600).contains(response.response.statusCode) {
if let decoder = WPXMLRPCDecoder(data: response.body), decoder.isFault() {
// when XML-RPC is disabled for authenticated calls (e.g. xmlrpc_enabled is false on WP.org),
// it will return a valid fault payload with a non-200
return .failure(.endpointError(.init(response: response, code: decoder.faultCode(), message: decoder.faultString())))
} else {
return .failure(.unacceptableStatusCode(response: response.response, body: response.body))
}
}

guard contentType.hasPrefix("application/xml") || contentType.hasPrefix("text/xml") else {
return .failure(.unparsableResponse(response: response.response, body: response.body, underlyingError: WordPressOrgXMLRPCApiError.unknown))
}

guard let decoder = WPXMLRPCDecoder(data: response.body) else {
return .failure(.unparsableResponse(response: response.response, body: response.body))
}

guard !decoder.isFault() else {
return .failure(.endpointError(.init(response: response, code: decoder.faultCode(), message: decoder.faultString())))
}

if let decoderError = decoder.error() {
return .failure(.unparsableResponse(response: response.response, body: response.body, underlyingError: decoderError))
}

guard let responseXML = decoder.object() else {
return .failure(.unparsableResponse(response: response.response, body: response.body))
Comment on lines +555 to +567
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: The case were the decoding fails because the decoder has no object and the case where the decoder could not be created call failure with the same error.

I don't think this is a major problem because I doubt at runtime we'll ever run into a case where decoder.error is nil and so is decoder.object.

}

return .success(HTTPAPIResponse(response: response.response, body: responseXML as AnyObject))
}
}

}

private extension WordPressAPIError where EndpointError == WordPressOrgXMLRPCApiFault {

/// Convert to NSError for backwards compatiblity.
///
/// Some Objective-C code in the WordPress app checks domain of the errors returned by `WordPressOrgXMLRPCApi`,
/// which can be WordPressOrgXMLRPCApiError or WPXMLRPCFaultErrorDomain.
///
/// Swift code should avoid dealing with NSError instances. Instead, they should use the strongly typed
/// `WordPressAPIError<WordPressOrgXMLRPCApiFault>`.
func asNSError() -> NSError {
let error: NSError
let data: Data?
let statusCode: Int?
switch self {
case let .requestEncodingFailure(underlyingError):
error = underlyingError as NSError
data = nil
statusCode = nil
case let .connection(urlError):
error = urlError as NSError
data = nil
statusCode = nil
case let .endpointError(fault):
error = NSError(domain: WPXMLRPCFaultErrorDomain, code: fault.code ?? 0, userInfo: [NSLocalizedDescriptionKey: fault.message].compactMapValues { $0 })
data = fault.response.body
statusCode = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this work?

Suggested change
statusCode = nil
statusCode = fault.response.statusCode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depending on what you mean by "work" 😄 . It sure makes sense to me to provide HTTP status code when it's available. However, in the existing implementation, only the status code is only set when it's 400 - 600. I'm just being super cautious to try keeping the existing behaviour changed when possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on what you mean by "work" 😄 .

Ahah. Good point @crazytonyli . Makes sense to be cautious 👍

Besides, ideally the HTTP level details should remain at the networking library level, and the app should only need to check the typed errors it gets, without needing to look into HTTP error codes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the app should only need to check the typed errors it gets, without needing to look into HTTP error codes.

Unfortunately that is not the case at the moment (here is an example), which is one of the reasons that I'd like to be cautious.

case let .unacceptableStatusCode(response, body):
error = WordPressOrgXMLRPCApiError.httpErrorStatusCode as NSError
data = body
statusCode = response.statusCode
case let .unparsableResponse(_, body, underlyingError):
error = underlyingError as NSError
data = body
statusCode = nil
case let .unknown(underlyingError):
error = underlyingError as NSError
data = nil
statusCode = nil
}

return WordPressOrgXMLRPCApi.convertError(error, data: data, statusCode: statusCode)
}

}
10 changes: 9 additions & 1 deletion WordPressKitTests/WordPressOrgXMLRPCApiTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,16 @@ class WordPressOrgXMLRPCApiTests: XCTestCase {
XCTAssertEqual(error.domain, WPXMLRPCFaultErrorDomain)
// 403 is the 'faultCode' in the HTTP response xml.
XCTAssertEqual(error.code, 403)
XCTAssertNil(error.userInfo[WordPressOrgXMLRPCApi.WordPressOrgXMLRPCApiErrorKeyData as String])
XCTAssertNil(error.userInfo[WordPressOrgXMLRPCApi.WordPressOrgXMLRPCApiErrorKeyStatusCode as String])

// The change highlights one difference between the existing Alamofire-backed API and the new
// URLSession-backed API: the error returned by the new one has an HTTP response body which is not
// the case exist in the old API. I think this is an acceptable change.
if WordPressOrgXMLRPCApi.useURLSession {
XCTAssertNotNil(error.userInfo[WordPressOrgXMLRPCApi.WordPressOrgXMLRPCApiErrorKeyData as String])
} else {
XCTAssertNil(error.userInfo[WordPressOrgXMLRPCApi.WordPressOrgXMLRPCApiErrorKeyData as String])
}
}
)
wait(for: [expect], timeout: 0.3)
Expand Down