Property value from an asynchronous operation

Question

I have an app which has a session mechanism. When a user creates an account in the app initially, the API returns an access token, the expiry date of that access token and a refresh token which can be used to retrieve a new access token when the existing access token expires.

I have written this singleton class to store these session related data.

import Foundation
import KeychainAccess
class SessionManager {
 static let shared = SessionManager()
 /// Save the access token string in the keychain.
 /// Returns the access token string if available.
 var accessToken: String? {
 set {
 let keychain = Keychain(service: Bundle.main.bundleIdentifier!)
 if let token = newValue {
 keychain["AccessToken"] = token
 } else {
 keychain["AccessToken"] = nil
 }
 }
 get {
 let keychain = Keychain(service: Bundle.main.bundleIdentifier!)
 do {
 if
 let accessToken = try keychain.getString("AccessToken"),
 let expiryDate = expiryDate {
 if expiryDate < Date() {
 if let refreshToken = refreshToken {
 return ApiClient.shared.refreshAccessToken(refreshToken: refreshToken)?.accessToken
 }
 return nil
 } else {
 return accessToken
 }
 } else {
 return nil
 }
 } catch {
 return nil
 }
 }
 }
 /// Save the refresh token in the keychain.
 /// Returns the refresh token string if available
 var refreshToken: String? {
 set {
 let keychain = Keychain(service: Bundle.main.bundleIdentifier!)
 if let token = newValue {
 keychain["RefreshToken"] = token
 } else {
 keychain["RefreshToken"] = nil
 }
 }
 get {
 let keychain = Keychain(service: Bundle.main.bundleIdentifier!)
 do {
 if let refreshToken = try keychain.getString("RefreshToken") {
 return refreshToken
 } else {
 return nil
 }
 } catch {
 return nil
 }
 }
 }
 /// Save the expiry date of access token in user defaults.
 /// Returns the expiry date of access token if available
 var expiryDate: Date? {
 set {
 if let date = newValue {
 UserDefaults.standard.set(date, forKey: "ExpiryDate")
 } else {
 UserDefaults.standard.removeObject(forKey: "ExpiryDate")
 }
 }
 get {
 if let expiryDate = UserDefaults.standard.object(forKey: "ExpiryDate") as? Date {
 return expiryDate
 } else {
 return nil
 }
 }
 }
}

The class has three computed properties; accessToken, refreshToken and expiryDate. I want to direct your attention to the accessToken property.

What I do here is whenever I retrieve this value in the app, I check if the access token is expired and if it is, I fire an API call with the refresh token to fetch a new access token. Now this is where things get a little hairy. I want to make the network call asynchronous. But since accessToken is a property, I cannot. So what I have done is, I'm making that API call synchronously. I'm using Alamofire in my project and I found this project called Alamofire-Synchronous that suits my need here. This is that method where the synchronous network call happens.

func refreshAccessToken(refreshToken: String) -> (accessToken: String, expiryDate: Date)? {
 let urlString = baseUrl + Endpoint.RefreshAccessToken
 let headers = [
 "Authorization": "Bearer \(refreshToken)"
 ]
 let response = Alamofire
 .request(urlString, method: .post, parameters: nil, encoding: JSONEncoding.default, headers: headers)
 .validate()
 .responseData()
 if let data = response.data {
 // parse dara and get retrieve accessToken and expiryDate
 // save them
 let sessionManager = SessionManager.shared
 sessionManager.accessToken = accessToken
 sessionManager.expiryDate = expiryDate
 return (accessToken, expiryDate)
 } else {
 if let httpError = response.result.error {
 print("HTTP Error: \(httpError)")
 return nil
 } else {
 return nil
 }
 }
}

This gets the job done. But I feel like having a synchronous network call is not a good idea. At the same time I don't just want to make this like an async function because I want to be able to get the accessToken value as a property anywhere from the app if needed. Is there a better, cleaner way to handle this?

Answer 1

In your accessToken property setter you've got the following statements:

set {
 let keychain = Keychain(service: Bundle.main.bundleIdentifier!)
 if let token = newValue {
 keychain["AccessToken"] = token
 } else {
 keychain["AccessToken"] = nil
 }
}

Since you're not using newValue for anything but to update the keychain, this can be reduced to:

set {
 let keychain = Keychain(service: Bundle.main.bundleIdentifier!)
 keychain["AccessToken"] = newValue
}

Same goes for your expiryDate getter:

get {
 if let expiryDate = UserDefaults.standard.object(forKey: "ExpiryDate") as? Date {
 return expiryDate
 } else {
 return nil
 }
}

Can be reduced to:

get {
 return UserDefaults.standard.object(forKey: "ExpiryDate") as? Date
}

Similarly, you can use try? as a shortcut if you want a variable to be set as nil if an error throws as you try to assign it, then you don't need the do-catch statements.

This is happening in a few places in your method, and in all the cases I see the alternative is both more concise, and more clear so I'd recommend taking that approach.

I also find the accessToken getter a bit hard to parse, and I'm not even 100% sure that it's functioning as expected.

if
 let accessToken = try keychain.getString("AccessToken"),
 let expiryDate = expiryDate {
 if expiryDate < Date() {
 if let refreshToken = refreshToken {
 return ApiClient.shared.refreshAccessToken(refreshToken: refreshToken)?.accessToken
 }
 return nil
 }
 else {
 return accessToken
 }
} else {
 return nil
}

Here's my take on it which breaks out preconditions with a guard statement, which has the same outcome if using if-let but is more expressive in terms of the purpose of the check.

Either way, guard or if-let, nested logic is greatly improved with a few brief comments to help guide readers through what's happening:

guard let accessToken = try? keychain.getString("AccessToken"),
 let expiryDate = expiryDate,
 let refreshToken = refreshToken
 else { return nil }
if expiryDate < Date() {
 // not expired, return refreshed access token
 return ApiClient.shared.refreshAccessToken(refreshToken: refreshToken)?.accessToken
}
else {
 // expired, return nil
 return nil
}

I agree with Gaurav that the synchronous request isn't a good idea (unless all of this is happening on a background thread). Maybe you could switch your approach for managing the accessToken from a computed property to using function that returns a closure:

// original 
var accessToken: String? { ... }
// alternative signatures
func getAccessToken() -> (String?) -> () { ... }
func setAccessToken(String) { ... }

You're also grabbing Keychain(service: Bundle.main.bundleIdentifier!) multiple times, so that might be a good candidate to set that as a property in the class. All of these changes combined your SessionManager class would look something like this:

import Foundation
import KeychainAccess
class SessionManager {
 static let shared = SessionManager()
 var keychain = Keychain(service: Bundle.main.bundleIdentifier!)
 /// Save the access token string in the keychain.
 func setAccessToken(token: String) {
 keychain["AccessToken"] = token
 }
 /// Returns the access token string if available.
 var accessToken: String? {
 guard let accessToken = try? keychain.getString("AccessToken"),
 let expiryDate = expiryDate,
 let refreshToken = refreshToken
 else { return nil }
 if expiryDate < Date() {
 // not expired, return refreshed access token
 return ApiClient.shared.refreshAccessToken(refreshToken: refreshToken)?.accessToken
 }
 else {
 // expired, return nil
 return nil
 }
 }
 /// Save the refresh token in the keychain.
 /// Returns the refresh token string if available
 var refreshToken: String? {
 set {
 keychain["RefreshToken"] = newValue
 }
 get {
 return try? keychain.getString("RefreshToken")
 }
 }
 /// Save the expiry date of access token in user defaults.
 /// Returns the expiry date of access token if available
 var expiryDate: Date? {
 set {
 guard let date = newValue else {
 UserDefaults.standard.removeObject(forKey: "ExpiryDate")
 return
 }
 UserDefaults.standard.set(date, forKey: "ExpiryDate")
 }
 get {
 return UserDefaults.standard.object(forKey: "ExpiryDate") as? Date
 }
 }
}

Answer 2

First of all, including a network call in property getter is not a good idea. Instead of, I would recommend you to replace this code in accessToken getter.

let expiryDate = expiryDate {
 if expiryDate < Date() {
 if let refreshToken = refreshToken {
 return ApiClient.shared.refreshAccessToken(refreshToken: refreshToken)?.accessToken
 }
 return nil
 }

to

let expiryDate = expiryDate {
 if expiryDate < Date() {
 return nil
 }

Before using accessToken check it for nil, if it is nil, refresh it using refreshToken. Convert ApiClient.shared.refreshAccessToken(refreshToken: refreshToken) to Asynchronous call.

• Isuru: I did think about this at first. But I use this accessToken property in many places throughout the app. Checking for nil in each place would result in a lot of code duplication. That's why I didn't do it.