-
Notifications
You must be signed in to change notification settings - Fork 423
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
@W-16589742: [iOS] REST wrappers for select SFAP APIs #3801
base: dev
Are you sure you want to change the base?
@W-16589742: [iOS] REST wrappers for select SFAP APIs #3801
Conversation
// Guards. | ||
guard | ||
let userAccountCurrent = UserAccountManager.shared.currentUserAccount, | ||
let restClient = RestClient.restClient(for: userAccountCurrent) else { |
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.
Why is this getting a RestClient based on the current user vs using the RestClient from init?
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.
Good catch. This guard lived somewhere else before I brought it to this file and I forgot to adjust it to scope. Thanks!
let restClient = RestClient.restClient(for: userAccountCurrent) else { | ||
throw sfapApiErrorWithMessage("Cannot invoke sfap_api client without a current user account.")} | ||
|
||
guard let modelNameUnwrapped = modelName else { |
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.
if you don't want to rename it, I think you could also do guard let modelName {
here
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.
👍🏻
do { | ||
return try sfapApiGenerationsResponse.asDecodable(type: SfapApiGenerationsResponseBody.self) | ||
} catch let error { | ||
throw sfapApiErrorWithMessage("Cannot JSON decode sfap_api generations response body due to a decoding error with description '\(error.localizedDescription)'.") |
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'm a little confused with the error handling, why does the error need to be caught and re-thrown?
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 like a re-throw here, also. I'll get that in. Thanks!
/// @param messageCode The `sfap_api` error code | ||
/// @param source The original `sfap_api` error response body | ||
@objc | ||
public class SfapApiError : NSError, @unchecked Sendable { |
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.
Why does this use NSError? Was there an issue with Swift's error type?
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.
Looks like it'd be alright with Swift's error type. I also made some matching changes I used on Android to make the sfap_api errorCode, message, messageCode and the original error response JSON available to the caller. Those were all pull request feedback items on that side, so hopefully those are valuable as matching developer experience here for iOS. Here's a screenshot of what catching an sfap_api error response looks like in the AppDelegate of iOSNativeSwiftTemplate. This specific case shows what would happen in the request had a malformed parameter, for example. sfap_api has a detailed response for that.
underlyingError: _, | ||
urlResponse: _ | ||
): if let errorResponseData = response as? Data { | ||
let sfapApiErrorResponseBody = try JSONDecoder().decode(SfapApiErrorResponseBody.self, from: errorResponseData) |
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.
@bbirman - I noticed we have an Error enum here on the iOS side. It looks like we could use .apiFailed
as a chance to receive the Data
and decode that to the documented error structure in the sfap_api doc and surface any valuable feedback to the client. The other cases could just re-throw. How does that look? On the Android side, we did create a type for the error responses much like this including the original error response's JSON source.
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.
sounds good!
import Foundation | ||
|
||
@objc(SFApApiClient) | ||
public class SfapApiClient : NSObject { |
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.
not important for the draft but I'm wondering if "Api" should be all caps to match SFRestAPI
🧱 Draft: Only one of the four endpoints is included. We'll review that for a quick look, then add the remaining three 🚧
This adds REST API clients for select sfap_api endpoints to match the Android version in forcedotcom/SalesforceMobileSDK-Android#2644
Only one of the four is included in the draft status. Here's a code sample from how I tested in iOSNativeSwiftTemplate's AppDelegate.