Use URLSession to send XMLRPC requests #719
Conversation
crazytonyli
force-pushed
the
wordpress-xmlrpc-api-error-new-impl
branch
from
e617373 to
8829f1a
Compare
crazytonyli
force-pushed
the
wordpress-xmlrpc-api-error-new-impl
branch
from
8829f1a to
962d56d
Compare
| // URLSession-backed API: the error returned by the new one may has HTTP response body which may not | ||
| // be the case exist in the old API. I think this is an acceptable change. |
There was a problem hiding this comment.
| // URLSession-backed API: the error returned by the new one may has HTTP response body which may not | |
| // be the case exist in the old API. I think this is an acceptable change. | |
| // URLSession-backed API: the error returned by the new one has an HTTP response body which is not | |
| // the case in the old API. I think this is an acceptable change. |
I think it's an acceptable change, too. In particular because the new way has more information than the old. If it was the other way around, then one would have to consider if the loss of information is dangerous for the client. But luckily that's not our case 😄
There was a problem hiding this comment.
I thought about suggesting to duplicated the tests and have one version run with useURLSession true and the other with false.
This might be achieved without lots of copy-paste by running each test via something like [true, false].each { useURLSession in ... }.
But, I expect using URLSession will soon become the only way to use the API. In which case, we might as well save the time to modify the tests and move on with the Alamofire removal work.
| /// Unlike the closed-based APIs, this method returns a concrete error type. You should consider handle the errors | ||
| /// as they are, instead of casing them to `NSError` instance. But in case you do need to cast them to `NSError`, | ||
| /// considering using the `asNSError` function if you need backward compatiblity with existing code, |
There was a problem hiding this comment.
| /// Unlike the closed-based APIs, this method returns a concrete error type. You should consider handle the errors | |
| /// as they are, instead of casing them to `NSError` instance. But in case you do need to cast them to `NSError`, | |
| /// considering using the `asNSError` function if you need backward compatiblity with existing code, | |
| /// 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. |
I only noticed the typo in compatibility when I loaded the text in this textarea in GitHub and the spell checker picked it up.
There was a problem hiding this comment.
🙈 Thanks! Updated in bcf8ea9
| /// considering using the `asNSError` function if you need backward compatiblity with existing code, | ||
| /// | ||
| /// - Parameters: | ||
| /// - streaming: set to true if there are large data (i.e. uploading files) in given `parameters`. `false` by default. |
There was a problem hiding this comment.
| /// - streaming: set to true if there are large data (i.e. uploading files) in given `parameters`. `false` by default. | |
| /// - streaming: set to `true` if there are large data (i.e. uploading files) in given `parameters`. `false` by default. |
"false" at the end is code fenced, so "true" should be, too.
| 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 |
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
| var response: HTTPAPIResponse<Data> | ||
|
|
||
| var code: Int? | ||
| var message: String? |
There was a problem hiding this comment.
Do these need to be mutable?
There was a problem hiding this comment.
Addressed in c4f376d
| } | ||
| } | ||
|
|
||
| if ["application/xml", "text/xml"].filter({ (type) -> Bool in return contentType.hasPrefix(type)}).count == 0 { |
There was a problem hiding this comment.
| if ["application/xml", "text/xml"].filter({ (type) -> Bool in return contentType.hasPrefix(type)}).count == 0 { | |
| if ["application/xml", "text/xml"].filter({ (type) -> Bool in return contentType.hasPrefix(type)}).isEmpty { |
From SwiftLint, I learned that isEmpty is more efficient than count == 0. I'm pretty sure the difference is not something we would notice at runtime, but I still think it's a good practice to use isEmpty.
There was a problem hiding this comment.
I have simplified this code in aeb9bec
| 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)) |
There was a problem hiding this comment.
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.
| case let .endpointError(fault): | ||
| error = NSError(domain: WPXMLRPCFaultErrorDomain, code: fault.code ?? 0, userInfo: [NSLocalizedDescriptionKey: fault.message].compactMapValues { $0 }) | ||
| data = fault.response.body | ||
| statusCode = nil |
There was a problem hiding this comment.
Would this work?
| statusCode = nil | |
| statusCode = fault.response.statusCode |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Description
This PR updates the XMLRPC API client to send HTTP requests using URLSession, when
useURLSessionis set to true (it's false by default).Error handling
This PR also changes the error handling in XMLPRC API and the API it surfaces.
When using Alamofire, in most cases, the XMLRPC API client returns
NSErrorinstances (plain and simple NSError instance, not the ones that are cast from Swift errors). It may returnURLErrortoo. It's quite different from the WP.com REST API client, which returnsNSErrorinstances that are cast from Swift errors, which is why I implemented a custom NSError bridge logic.Since there is no "bridging" happening in
WordPressOrgXMLRPCApi, I didn't reuse the custom error bridging code. Instead, I re-used the existing function that creates newNSErrorinstances, for the exiting closure-based APIs. The new async functioncall(method:parameters:...)returnsWordPressAPIError<WordPressOrgXMLRPCApiFault>error instances. That means, depending on which method you call, you'll get different kinds of errors.The new implementation is in the
decodeXMLRPCResultandasNSErrorfunctions, which together should work exactly like the existinghandleResponseWithDatafunction. And this "non-change" should be covered by the existing error case unit tests.Testing Details
Similar to #720, the unit tests pass when
useURLSessionis set to true.wordpress-mobile/WordPress-iOS#22540 can be used to get an app build which enables
useURLSessionby default.CHANGELOG.mdif necessary.