From 8973adbdbdc9cb85cf9c3f1b0a1e3a6a3816f899 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kiel=20Gillard=20=F0=9F=A4=AA?= Date: Sun, 19 Apr 2020 08:31:20 +1000 Subject: [PATCH 1/2] Add lastCredentialRenewTime and honour it in FeedlyRefreshAccessTokenOperation --- Frameworks/Account/AccountMetadata.swift | 11 ++++++++++ .../FeedlyRefreshAccessTokenOperation.swift | 21 ++++++++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/Frameworks/Account/AccountMetadata.swift b/Frameworks/Account/AccountMetadata.swift index 7c5f378f9..f4c50ab44 100644 --- a/Frameworks/Account/AccountMetadata.swift +++ b/Frameworks/Account/AccountMetadata.swift @@ -23,6 +23,7 @@ final class AccountMetadata: Codable { case lastArticleFetchStartTime = "lastArticleFetch" case lastArticleFetchEndTime case endpointURL + case lastCredentialRenewTime = "lastCredentialRenewTime" } var name: String? { @@ -80,6 +81,16 @@ final class AccountMetadata: Codable { } } } + + /// The last moment an account successfully renewed its credentials, or `nil` if no such moment exists. + /// An account delegate can use this value to decide when to next ask the service provider to renew credentials. + var lastCredentialRenewTime: Date? { + didSet { + if lastCredentialRenewTime != oldValue { + valueDidChange(.lastCredentialRenewTime) + } + } + } weak var delegate: AccountMetadataDelegate? diff --git a/Frameworks/Account/Feedly/Operations/FeedlyRefreshAccessTokenOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlyRefreshAccessTokenOperation.swift index d1c68d970..d6412f202 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlyRefreshAccessTokenOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlyRefreshAccessTokenOperation.swift @@ -17,14 +17,31 @@ final class FeedlyRefreshAccessTokenOperation: FeedlyOperation { let account: Account let log: OSLog - init(account: Account, service: OAuthAccessTokenRefreshing, oauthClient: OAuthAuthorizationClient, log: OSLog) { + /// The moment the refresh is being requested. The token will refresh only if the account's `lastCredentialRenewTime` is not on the same day as this moment. When nil, the operation will always refresh the token. + let refreshDate: Date? + + init(account: Account, service: OAuthAccessTokenRefreshing, oauthClient: OAuthAuthorizationClient, refreshDate: Date?, log: OSLog) { self.oauthClient = oauthClient self.service = service self.account = account + self.refreshDate = refreshDate self.log = log } override func run() { + // Only refresh the token if these dates are not on the same day. + let shouldRefresh: Bool = { + guard let date = refreshDate, let lastRenewDate = account.metadata.lastCredentialRenewTime else { + return true + } + return !Calendar.current.isDate(lastRenewDate, equalTo: date, toGranularity: .day) + }() + + guard shouldRefresh else { + didFinish() + return + } + let refreshToken: Credentials do { @@ -64,6 +81,8 @@ final class FeedlyRefreshAccessTokenOperation: FeedlyOperation { // Now store the access token because we want the account delegate to use it. try account.storeCredentials(grant.accessToken) + account.metadata.lastCredentialRenewTime = Date() + didFinish() } catch { didFinish(with: error) From 5ab13ae705ba75540bf3947411ee0a7c9e44d2fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kiel=20Gillard=20=F0=9F=A4=AA?= Date: Sun, 19 Apr 2020 08:45:51 +1000 Subject: [PATCH 2/2] Ensure token is refreshed at least once a day before syncing. --- .../Feedly/FeedlyAccountDelegate.swift | 26 ++++++++++++------- .../FeedlyAddNewFeedOperation.swift | 2 +- .../FeedlyGetUpdatedArticleIdsOperation.swift | 4 +-- ...edlyIngestStarredArticleIdsOperation.swift | 4 +-- ...eedlyIngestStreamArticleIdsOperation.swift | 4 +-- ...eedlyIngestUnreadArticleIdsOperation.swift | 4 +-- .../FeedlyRefreshAccessTokenOperation.swift | 1 + .../Operations/FeedlySyncAllOperation.swift | 16 ++++++------ 8 files changed, 34 insertions(+), 27 deletions(-) diff --git a/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift b/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift index 369e237f8..184ea576b 100644 --- a/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift +++ b/Frameworks/Account/Feedly/FeedlyAccountDelegate.swift @@ -111,11 +111,20 @@ final class FeedlyAccountDelegate: AccountDelegate { } let log = self.log - let operation = FeedlySyncAllOperation(account: account, credentials: credentials, caller: caller, database: database, lastSuccessfulFetchStartDate: accountMetadata?.lastArticleFetchStartTime, downloadProgress: refreshProgress, log: log) - operation.downloadProgress = refreshProgress + let refreshAccessToken = FeedlyRefreshAccessTokenOperation(account: account, service: self, oauthClient: oauthAuthorizationClient, refreshDate: Date(), log: log) + refreshAccessToken.downloadProgress = refreshProgress + operationQueue.add(refreshAccessToken) + + let syncAllOperation = FeedlySyncAllOperation(account: account, feedlyUserId: credentials.username, caller: caller, database: database, lastSuccessfulFetchStartDate: accountMetadata?.lastArticleFetchStartTime, downloadProgress: refreshProgress, log: log) + + syncAllOperation.downloadProgress = refreshProgress + + // Ensure the sync uses the latest credential. + syncAllOperation.addDependency(refreshAccessToken) + let date = Date() - operation.syncCompletionHandler = { [weak self] result in + syncAllOperation.syncCompletionHandler = { [weak self] result in if case .success = result { self?.accountMetadata?.lastArticleFetchStartTime = date self?.accountMetadata?.lastArticleFetchEndTime = Date() @@ -125,9 +134,9 @@ final class FeedlyAccountDelegate: AccountDelegate { completion(result) } - currentSyncAllOperation = operation + currentSyncAllOperation = syncAllOperation - operationQueue.add(operation) + operationQueue.add(syncAllOperation) } func sendArticleStatus(for account: Account, completion: @escaping ((Result) -> Void)) { @@ -155,7 +164,7 @@ final class FeedlyAccountDelegate: AccountDelegate { let group = DispatchGroup() - let ingestUnread = FeedlyIngestUnreadArticleIdsOperation(account: account, credentials: credentials, service: caller, database: database, newerThan: nil, log: log) + let ingestUnread = FeedlyIngestUnreadArticleIdsOperation(account: account, userId: credentials.username, service: caller, database: database, newerThan: nil, log: log) group.enter() ingestUnread.completionBlock = { _ in @@ -163,7 +172,7 @@ final class FeedlyAccountDelegate: AccountDelegate { } - let ingestStarred = FeedlyIngestStarredArticleIdsOperation(account: account, credentials: credentials, service: caller, database: database, newerThan: nil, log: log) + let ingestStarred = FeedlyIngestStarredArticleIdsOperation(account: account, userId: credentials.username, service: caller, database: database, newerThan: nil, log: log) group.enter() ingestStarred.completionBlock = { _ in @@ -492,9 +501,6 @@ final class FeedlyAccountDelegate: AccountDelegate { func accountDidInitialize(_ account: Account) { credentials = try? account.retrieveCredentials(type: .oauthAccessToken) - - let refreshAccessToken = FeedlyRefreshAccessTokenOperation(account: account, service: self, oauthClient: oauthAuthorizationClient, log: log) - operationQueue.add(refreshAccessToken) } func accountWillBeDeleted(_ account: Account) { diff --git a/Frameworks/Account/Feedly/Operations/FeedlyAddNewFeedOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlyAddNewFeedOperation.swift index e3641511e..d4852c2f4 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlyAddNewFeedOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlyAddNewFeedOperation.swift @@ -89,7 +89,7 @@ class FeedlyAddNewFeedOperation: FeedlyOperation, FeedlyOperationDelegate, Feedl createFeeds.downloadProgress = downloadProgress operationQueue.add(createFeeds) - let syncUnread = FeedlyIngestUnreadArticleIdsOperation(account: account, credentials: credentials, service: syncUnreadIdsService, database: database, newerThan: nil, log: log) + let syncUnread = FeedlyIngestUnreadArticleIdsOperation(account: account, userId: credentials.username, service: syncUnreadIdsService, database: database, newerThan: nil, log: log) syncUnread.addDependency(createFeeds) syncUnread.downloadProgress = downloadProgress syncUnread.delegate = self diff --git a/Frameworks/Account/Feedly/Operations/FeedlyGetUpdatedArticleIdsOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlyGetUpdatedArticleIdsOperation.swift index 1ed7a2c10..43ce44643 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlyGetUpdatedArticleIdsOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlyGetUpdatedArticleIdsOperation.swift @@ -29,8 +29,8 @@ class FeedlyGetUpdatedArticleIdsOperation: FeedlyOperation, FeedlyEntryIdentifie self.log = log } - convenience init(account: Account, credentials: Credentials, service: FeedlyGetStreamIdsService, newerThan: Date?, log: OSLog) { - let all = FeedlyCategoryResourceId.Global.all(for: credentials.username) + convenience init(account: Account, userId: String, service: FeedlyGetStreamIdsService, newerThan: Date?, log: OSLog) { + let all = FeedlyCategoryResourceId.Global.all(for: userId) self.init(account: account, resource: all, service: service, newerThan: newerThan, log: log) } diff --git a/Frameworks/Account/Feedly/Operations/FeedlyIngestStarredArticleIdsOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlyIngestStarredArticleIdsOperation.swift index 02dc42043..7793315e0 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlyIngestStarredArticleIdsOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlyIngestStarredArticleIdsOperation.swift @@ -25,8 +25,8 @@ final class FeedlyIngestStarredArticleIdsOperation: FeedlyOperation { private var remoteEntryIds = Set() private let log: OSLog - convenience init(account: Account, credentials: Credentials, service: FeedlyGetStreamIdsService, database: SyncDatabase, newerThan: Date?, log: OSLog) { - let resource = FeedlyTagResourceId.Global.saved(for: credentials.username) + convenience init(account: Account, userId: String, service: FeedlyGetStreamIdsService, database: SyncDatabase, newerThan: Date?, log: OSLog) { + let resource = FeedlyTagResourceId.Global.saved(for: userId) self.init(account: account, resource: resource, service: service, database: database, newerThan: newerThan, log: log) } diff --git a/Frameworks/Account/Feedly/Operations/FeedlyIngestStreamArticleIdsOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlyIngestStreamArticleIdsOperation.swift index 12b906d12..8e7716159 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlyIngestStreamArticleIdsOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlyIngestStreamArticleIdsOperation.swift @@ -28,8 +28,8 @@ class FeedlyIngestStreamArticleIdsOperation: FeedlyOperation { self.log = log } - convenience init(account: Account, credentials: Credentials, service: FeedlyGetStreamIdsService, log: OSLog) { - let all = FeedlyCategoryResourceId.Global.all(for: credentials.username) + convenience init(account: Account, userId: String, service: FeedlyGetStreamIdsService, log: OSLog) { + let all = FeedlyCategoryResourceId.Global.all(for: userId) self.init(account: account, resource: all, service: service, log: log) } diff --git a/Frameworks/Account/Feedly/Operations/FeedlyIngestUnreadArticleIdsOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlyIngestUnreadArticleIdsOperation.swift index 669a9672d..271caec65 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlyIngestUnreadArticleIdsOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlyIngestUnreadArticleIdsOperation.swift @@ -26,8 +26,8 @@ final class FeedlyIngestUnreadArticleIdsOperation: FeedlyOperation { private var remoteEntryIds = Set() private let log: OSLog - convenience init(account: Account, credentials: Credentials, service: FeedlyGetStreamIdsService, database: SyncDatabase, newerThan: Date?, log: OSLog) { - let resource = FeedlyCategoryResourceId.Global.all(for: credentials.username) + convenience init(account: Account, userId: String, service: FeedlyGetStreamIdsService, database: SyncDatabase, newerThan: Date?, log: OSLog) { + let resource = FeedlyCategoryResourceId.Global.all(for: userId) self.init(account: account, resource: resource, service: service, database: database, newerThan: newerThan, log: log) } diff --git a/Frameworks/Account/Feedly/Operations/FeedlyRefreshAccessTokenOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlyRefreshAccessTokenOperation.swift index d6412f202..b4878dc49 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlyRefreshAccessTokenOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlyRefreshAccessTokenOperation.swift @@ -38,6 +38,7 @@ final class FeedlyRefreshAccessTokenOperation: FeedlyOperation { }() guard shouldRefresh else { + os_log(.debug, log: log, "Skipping access token renewal.") didFinish() return } diff --git a/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift b/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift index b0c87e027..6a2e36a76 100644 --- a/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift +++ b/Frameworks/Account/Feedly/Operations/FeedlySyncAllOperation.swift @@ -33,7 +33,7 @@ final class FeedlySyncAllOperation: FeedlyOperation { /// /// Download articles for statuses at the union of those statuses without its corresponding article and those included in 3 (changed since last successful sync). /// - init(account: Account, credentials: Credentials, lastSuccessfulFetchStartDate: Date?, markArticlesService: FeedlyMarkArticlesService, getUnreadService: FeedlyGetStreamIdsService, getCollectionsService: FeedlyGetCollectionsService, getStreamContentsService: FeedlyGetStreamContentsService, getStarredService: FeedlyGetStreamIdsService, getStreamIdsService: FeedlyGetStreamIdsService, getEntriesService: FeedlyGetEntriesService, database: SyncDatabase, downloadProgress: DownloadProgress, log: OSLog) { + init(account: Account, feedlyUserId: String, lastSuccessfulFetchStartDate: Date?, markArticlesService: FeedlyMarkArticlesService, getUnreadService: FeedlyGetStreamIdsService, getCollectionsService: FeedlyGetCollectionsService, getStreamContentsService: FeedlyGetStreamContentsService, getStarredService: FeedlyGetStreamIdsService, getStreamIdsService: FeedlyGetStreamIdsService, getEntriesService: FeedlyGetEntriesService, database: SyncDatabase, downloadProgress: DownloadProgress, log: OSLog) { self.syncUUID = UUID() self.log = log self.operationQueue.suspend() @@ -53,7 +53,7 @@ final class FeedlySyncAllOperation: FeedlyOperation { getCollections.delegate = self getCollections.downloadProgress = downloadProgress getCollections.addDependency(sendArticleStatuses) - self.operationQueue.add(getCollections) + self.operationQueue.add(getCollections) // Ensure a folder exists for each Collection, removing Folders without a corresponding Collection. let mirrorCollectionsAsFolders = FeedlyMirrorCollectionsAsFoldersOperation(account: account, collectionsProvider: getCollections, log: log) @@ -67,14 +67,14 @@ final class FeedlySyncAllOperation: FeedlyOperation { createFeedsOperation.addDependency(mirrorCollectionsAsFolders) self.operationQueue.add(createFeedsOperation) - let getAllArticleIds = FeedlyIngestStreamArticleIdsOperation(account: account, credentials: credentials, service: getStreamIdsService, log: log) + let getAllArticleIds = FeedlyIngestStreamArticleIdsOperation(account: account, userId: feedlyUserId, service: getStreamIdsService, log: log) getAllArticleIds.delegate = self getAllArticleIds.downloadProgress = downloadProgress getAllArticleIds.addDependency(createFeedsOperation) self.operationQueue.add(getAllArticleIds) // Get each page of unread article ids in the global.all stream for the last 31 days (nil = Feedly API default). - let getUnread = FeedlyIngestUnreadArticleIdsOperation(account: account, credentials: credentials, service: getUnreadService, database: database, newerThan: nil, log: log) + let getUnread = FeedlyIngestUnreadArticleIdsOperation(account: account, userId: feedlyUserId, service: getUnreadService, database: database, newerThan: nil, log: log) getUnread.delegate = self getUnread.addDependency(getAllArticleIds) getUnread.downloadProgress = downloadProgress @@ -82,14 +82,14 @@ final class FeedlySyncAllOperation: FeedlyOperation { // Get each page of the article ids which have been update since the last successful fetch start date. // If the date is nil, this operation provides an empty set (everything is new, nothing is updated). - let getUpdated = FeedlyGetUpdatedArticleIdsOperation(account: account, credentials: credentials, service: getStreamIdsService, newerThan: lastSuccessfulFetchStartDate, log: log) + let getUpdated = FeedlyGetUpdatedArticleIdsOperation(account: account, userId: feedlyUserId, service: getStreamIdsService, newerThan: lastSuccessfulFetchStartDate, log: log) getUpdated.delegate = self getUpdated.downloadProgress = downloadProgress getUpdated.addDependency(createFeedsOperation) self.operationQueue.add(getUpdated) // Get each page of the article ids for starred articles. - let getStarred = FeedlyIngestStarredArticleIdsOperation(account: account, credentials: credentials, service: getStarredService, database: database, newerThan: nil, log: log) + let getStarred = FeedlyIngestStarredArticleIdsOperation(account: account, userId: feedlyUserId, service: getStarredService, database: database, newerThan: nil, log: log) getStarred.delegate = self getStarred.downloadProgress = downloadProgress getStarred.addDependency(createFeedsOperation) @@ -125,8 +125,8 @@ final class FeedlySyncAllOperation: FeedlyOperation { self.operationQueue.add(finishOperation) } - convenience init(account: Account, credentials: Credentials, caller: FeedlyAPICaller, database: SyncDatabase, lastSuccessfulFetchStartDate: Date?, downloadProgress: DownloadProgress, log: OSLog) { - self.init(account: account, credentials: credentials, lastSuccessfulFetchStartDate: lastSuccessfulFetchStartDate, markArticlesService: caller, getUnreadService: caller, getCollectionsService: caller, getStreamContentsService: caller, getStarredService: caller, getStreamIdsService: caller, getEntriesService: caller, database: database, downloadProgress: downloadProgress, log: log) + convenience init(account: Account, feedlyUserId: String, caller: FeedlyAPICaller, database: SyncDatabase, lastSuccessfulFetchStartDate: Date?, downloadProgress: DownloadProgress, log: OSLog) { + self.init(account: account, feedlyUserId: feedlyUserId, lastSuccessfulFetchStartDate: lastSuccessfulFetchStartDate, markArticlesService: caller, getUnreadService: caller, getCollectionsService: caller, getStreamContentsService: caller, getStarredService: caller, getStreamIdsService: caller, getEntriesService: caller, database: database, downloadProgress: downloadProgress, log: log) } override func run() {