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

Make WordPressOrgXMLRPCApiError conform to CustomNSError #790

Open
wants to merge 7 commits into
base: trunk
Choose a base branch
from
18 changes: 18 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,24 @@ _None._

-->

## Unreleased

### Breaking Changes

- The Objective-C-visible `WordPressOrgXMLRPCError` `enum` has been renamed to `WordPressOrgXMLRPCErrorCode`, `WordPressOrgXMLRPCError.Code` in Swift [#790]

### New Features

_None._

### Bug Fixes

_None._

### Internal Changes

_None._

## 17.0.0

### Breaking Changes
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#import <Foundation/Foundation.h>

/// Error domain of `NSError` instances that are converted from `WordPressOrgXMLRPCApiError`
/// and `WordPressAPIError<WordPressOrgXMLRPCApiError>` instances.
///
/// This matches the compiler generated value and is used to ensure consistent error domain across error types and SPM or Framework build modes.
///
/// See `extension WordPressComRestApiEndpointError: CustomNSError` in CoreAPI package for context.
static NSString *const _Nonnull WordPressOrgXMLRPCApiErrorDomain = @"WordPressKit.WordPressOrgXMLRPCApiError";
53 changes: 16 additions & 37 deletions Sources/CoreAPI/WordPressOrgXMLRPCApi.swift
Original file line number Diff line number Diff line change
Expand Up @@ -288,38 +288,6 @@ private class SessionDelegate: NSObject, URLSessionDelegate {
}
}
}

/// Error constants for the WordPress XML-RPC API
@objc public enum WordPressOrgXMLRPCApiError: Int, Error {
/// An error HTTP status code was returned.
case httpErrorStatusCode
/// The serialization of the request failed.
case requestSerializationFailed
/// The serialization of the response failed.
case responseSerializationFailed
/// An unknown error occurred.
case unknown
}

extension WordPressOrgXMLRPCApiError: LocalizedError {
public var errorDescription: String? {
return NSLocalizedString("There was a problem communicating with the site.", comment: "A general error message shown to the user when there was an API communication failure.")
}

public var failureReason: String? {
switch self {
case .httpErrorStatusCode:
return NSLocalizedString("An HTTP error code was returned.", comment: "A failure reason for when an error HTTP status code was returned from the site.")
case .requestSerializationFailed:
return NSLocalizedString("The serialization of the request failed.", comment: "A failure reason for when the request couldn't be serialized.")
case .responseSerializationFailed:
return NSLocalizedString("The serialization of the response failed.", comment: "A failure reason for when the response couldn't be serialized.")
case .unknown:
return NSLocalizedString("An unknown error occurred.", comment: "A failure reason for when the error that occured wasn't able to be determined.")
}
}
}

public struct WordPressOrgXMLRPCApiFault: LocalizedError, HTTPURLResponseProviding {
public var response: HTTPAPIResponse<Data>
public let code: Int?
Expand Down Expand Up @@ -347,7 +315,13 @@ private extension WordPressAPIResult<HTTPAPIResponse<Data>, WordPressOrgXMLRPCAp
// 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))
return .failure(
.unparsableResponse(
response: response.response,
body: response.body,
underlyingError: WordPressOrgXMLRPCApiError(code: .unknown)
)
)
}

if (400..<600).contains(response.response.statusCode) {
Expand All @@ -361,7 +335,13 @@ private extension WordPressAPIResult<HTTPAPIResponse<Data>, WordPressOrgXMLRPCAp
}

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

guard let decoder = WPXMLRPCDecoder(data: response.body) else {
Expand Down Expand Up @@ -391,7 +371,7 @@ private extension WordPressAPIError where EndpointError == WordPressOrgXMLRPCApi
/// 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.
/// which can be `WordPressOrgXMLRPCApiError` or `WPXMLRPCFaultErrorDomain`.
///
/// Swift code should avoid dealing with NSError instances. Instead, they should use the strongly typed
/// `WordPressAPIError<WordPressOrgXMLRPCApiFault>`.
Expand All @@ -413,7 +393,7 @@ private extension WordPressAPIError where EndpointError == WordPressOrgXMLRPCApi
data = fault.response.body
statusCode = nil
case let .unacceptableStatusCode(response, body):
error = WordPressOrgXMLRPCApiError.httpErrorStatusCode as NSError
error = WordPressOrgXMLRPCApiError(code: .httpErrorStatusCode) as NSError
data = body
statusCode = response.statusCode
case let .unparsableResponse(_, body, underlyingError):
Expand All @@ -428,5 +408,4 @@ private extension WordPressAPIError where EndpointError == WordPressOrgXMLRPCApi

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

}
14 changes: 14 additions & 0 deletions Sources/CoreAPI/WordPressOrgXMLRPCApiError+NSErrorBridge.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import Foundation
#if SWIFT_PACKAGE
import APIInterface
#endif

/// See `extension WordPressComRestApiEndpointError: CustomNSError` for documentation and rationale.
extension WordPressOrgXMLRPCApiError: CustomNSError {

public static let errorDomain = WordPressOrgXMLRPCApiErrorDomain

public var errorCode: Int { code.rawValue }

public var errorUserInfo: [String: Any] { [:] }
}
40 changes: 40 additions & 0 deletions Sources/CoreAPI/WordPressOrgXMLRPCApiError.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import Foundation

public struct WordPressOrgXMLRPCApiError: Error {

/// Error constants for the WordPress XML-RPC API
@objc public enum Code: Int, CaseIterable {
/// An error HTTP status code was returned.
case httpErrorStatusCode
/// The serialization of the request failed.
case requestSerializationFailed
/// The serialization of the response failed.
case responseSerializationFailed
/// An unknown error occurred.
case unknown
}

let code: Code
}
Comment on lines +3 to +18
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@crazytonyli I ended up having to wrap the error code enum into an Error type like you did for the REST API error.

Otherwise, between the @objc visibility (needed because WordPress-Authenticator uses it 🙄 ) and the Error conformance, I couldn't find a way to stop the compiler from automatically generating WordPressOrgXMLRPCApiErrorDomain for @objc public enum WordPressOrgXMLRPCApiError: Int, Error.

I think this is an okay solution. Apart for the fact that's the only one I could get to work, it's, as far as I can see, the same setup as the other error, which makes the implementation consistent.


extension WordPressOrgXMLRPCApiError: LocalizedError {
public var errorDescription: String? {
return NSLocalizedString(
"There was a problem communicating with the site.",
comment: "A general error message shown to the user when there was an API communication failure."
)
}

public var failureReason: String? {
switch code {
case .httpErrorStatusCode:
return NSLocalizedString("An HTTP error code was returned.", comment: "A failure reason for when an error HTTP status code was returned from the site.")
case .requestSerializationFailed:
return NSLocalizedString("The serialization of the request failed.", comment: "A failure reason for when the request couldn't be serialized.")
case .responseSerializationFailed:
return NSLocalizedString("The serialization of the response failed.", comment: "A failure reason for when the response couldn't be serialized.")
case .unknown:
return NSLocalizedString("An unknown error occurred.", comment: "A failure reason for when the error that occured wasn't able to be determined.")
}
}
}
1 change: 1 addition & 0 deletions Sources/WordPressKit/WordPressKit.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ FOUNDATION_EXPORT const unsigned char WordPressKitVersionString[];
#import <WordPressKit/WordPressComRESTAPIVersion.h>
#import <WordPressKit/WordPressComRESTAPIVersionedPathBuilder.h>
#import <WordPressKit/WordPressComRestApiErrorDomain.h>
#import <WordPressKit/WordPressOrgXMLRPCApiErrorDomain.h>

#import <WordPressKit/ServiceRemoteWordPressComREST.h>
#import <WordPressKit/ServiceRemoteWordPressXMLRPC.h>
Expand Down
1 change: 0 additions & 1 deletion Tests/CoreAPITests/WordPressComRestApiTests+Error.swift
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,4 @@ class WordPressComRestApiErrorTests: XCTestCase {
func testErrorDomain() {
XCTAssertEqual(WordPressComRestApiErrorDomain, WordPressComRestApiEndpointError.errorDomain)
}

}
25 changes: 25 additions & 0 deletions Tests/CoreAPITests/WordPressOrgXMLRPCApiErrorTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#if SWIFT_PACKAGE
import APIInterface
@testable import CoreAPI
#else
@testable import WordPressKit
#endif
import XCTest

class WordPressOrgXMLRPCApiErrorTests: XCTestCase {

func testNSErrorBridging() throws {
for error in WordPressOrgXMLRPCApiError.Code.allCases {
let xmlRPCError = try XCTUnwrap(WordPressOrgXMLRPCApiError(code: error))
let apiError = WordPressAPIError.endpointError(xmlRPCError)
let newNSError = apiError as NSError

XCTAssertEqual(newNSError.domain, "WordPressKit.WordPressOrgXMLRPCApiError")
XCTAssertEqual(newNSError.code, error.rawValue)
}
}

func testErrorDomain() {
XCTAssertEqual(WordPressOrgXMLRPCApiErrorDomain, WordPressOrgXMLRPCApiError.errorDomain)
}
}
24 changes: 5 additions & 19 deletions Tests/CoreAPITests/WordPressOrgXMLRPCApiTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import XCTest
import OHHTTPStubs
import wpxmlrpc
#if SWIFT_PACKAGE
import APIInterface
@testable import CoreAPI
import OHHTTPStubsSwift
#else
Expand Down Expand Up @@ -63,13 +64,8 @@ class WordPressOrgXMLRPCApiTests: XCTestCase {
},
failure: { (error, _) in
expect.fulfill()
// When building for SPM, the compiler doesn't generate the domain constant.
#if SWIFT_PACKAGE
XCTAssertEqual(error.domain, "CoreAPI.WordPressOrgXMLRPCApiError")
#else
XCTAssertEqual(error.domain, WordPressOrgXMLRPCApiErrorDomain)
#endif
XCTAssertEqual(error.code, WordPressOrgXMLRPCApiError.httpErrorStatusCode.rawValue)
XCTAssertEqual(error.code, WordPressOrgXMLRPCApiError.Code.httpErrorStatusCode.rawValue)
XCTAssertEqual(error.localizedFailureReason, "An HTTP error code 404 was returned.")
XCTAssertNotNil(error.userInfo[WordPressOrgXMLRPCApi.WordPressOrgXMLRPCApiErrorKeyData as String])
XCTAssertNotNil(error.userInfo[WordPressOrgXMLRPCApi.WordPressOrgXMLRPCApiErrorKeyStatusCode as String])
Expand All @@ -96,12 +92,7 @@ class WordPressOrgXMLRPCApiTests: XCTestCase {
expect.fulfill()

XCTAssertFalse(error is WordPressOrgXMLRPCApiError)
// When building for SPM, the compiler doesn't generate the domain constant.
#if SWIFT_PACKAGE
XCTAssertEqual(error.domain, "CoreAPI.WordPressOrgXMLRPCApiError")
#else
XCTAssertEqual(error.domain, WordPressOrgXMLRPCApiErrorDomain)
#endif
XCTAssertEqual(error.code, 403)
XCTAssertEqual(error.localizedFailureReason, "An HTTP error code 403 was returned.")
XCTAssertNotNil(error.userInfo[WordPressOrgXMLRPCApi.WordPressOrgXMLRPCApiErrorKeyData as String])
Expand All @@ -128,15 +119,10 @@ class WordPressOrgXMLRPCApiTests: XCTestCase {
failure: { (error, _) in
expect.fulfill()

XCTAssertTrue(error is WordPressOrgXMLRPCApiError)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only change in behavior from the rewrite.

The NSError-converted error (via asNSerror()) that the API passes to this failure branch doesn't convert back to WordPressOrgXMLRPCApiError.

Copy link
Contributor

Choose a reason for hiding this comment

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

I might have missed something. Does that mean the error in the failure block was WordPressOrgXMLRPCApiError, but in this PR it's changed to something else?

// When building for SPM, the compiler doesn't generate the domain constant.
#if SWIFT_PACKAGE
XCTAssertEqual(error.domain, "CoreAPI.WordPressOrgXMLRPCApiError")
#else
XCTAssertEqual(error.domain, WordPressOrgXMLRPCApiErrorDomain)
#endif
XCTAssertEqual(error.code, WordPressOrgXMLRPCApiError.unknown.rawValue)
XCTAssertEqual(error.localizedFailureReason, WordPressOrgXMLRPCApiError.unknown.failureReason)
let errorUnknonw = WordPressOrgXMLRPCApiError(code: .unknown)
XCTAssertEqual(error.code, errorUnknonw.code.rawValue)
XCTAssertEqual(error.localizedFailureReason, errorUnknonw.failureReason)
XCTAssertNotNil(error.userInfo[WordPressOrgXMLRPCApi.WordPressOrgXMLRPCApiErrorKeyData as String])
XCTAssertNil(error.userInfo[WordPressOrgXMLRPCApi.WordPressOrgXMLRPCApiErrorKeyStatusCode as String])
}
Expand Down
Loading