From aecf90f9f8618df6e7b0c9704342928a914a59ba Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Wed, 28 Oct 2020 16:16:15 -0500 Subject: [PATCH 1/8] Don't chunk entries at the caller level because it is already done in the delegate --- .../Account/ReaderAPI/ReaderAPICaller.swift | 49 +++++++------------ 1 file changed, 17 insertions(+), 32 deletions(-) diff --git a/Account/Sources/Account/ReaderAPI/ReaderAPICaller.swift b/Account/Sources/Account/ReaderAPI/ReaderAPICaller.swift index 782f22e0a..a3e4fe06d 100644 --- a/Account/Sources/Account/ReaderAPI/ReaderAPICaller.swift +++ b/Account/Sources/Account/ReaderAPI/ReaderAPICaller.swift @@ -576,42 +576,27 @@ final class ReaderAPICaller: NSObject { request.setValue("application/x-www-form-urlencoded", forHTTPHeaderField: "Content-Type") request.httpMethod = "POST" - let chunkedArticleIds = articleIDs.chunked(into: 200) - let group = DispatchGroup() - var groupEntries = [ReaderAPIEntry]() - var groupError: Error? = nil + // Get ids from above into hex representation of value + let idsToFetch = articleIDs.map({ (reference) -> String in + return "i=tag:google.com,2005:reader/item/\(reference)" + }).joined(separator:"&") - for articleIDChunk in chunkedArticleIds { - let itemFetchParameters = articleIDChunk.map({ articleID -> String in - return "i=tag:google.com,2005:reader/item/\(articleID)" - }).joined(separator:"&") - - let postData = "T=\(token)&output=json&\(itemFetchParameters)".data(using: String.Encoding.utf8) - - group.enter() - self.transport.send(request: request, method: HTTPMethod.post, data: postData!, resultType: ReaderAPIEntryWrapper.self, completion: { (result) in - switch result { - case .success(let (_, entryWrapper)): - guard let entryWrapper = entryWrapper else { - completion(.failure(ReaderAPIAccountDelegateError.invalidResponse)) - return - } - groupEntries.append(contentsOf: entryWrapper.entries) - group.leave() - case .failure(let error): - groupError = error - group.leave() + let postData = "T=\(token)&output=json&\(idsToFetch)".data(using: String.Encoding.utf8) + + self.transport.send(request: request, method: HTTPMethod.post, data: postData!, resultType: ReaderAPIEntryWrapper.self, completion: { (result) in + switch result { + case .success(let (_, entryWrapper)): + guard let entryWrapper = entryWrapper else { + completion(.failure(ReaderAPIAccountDelegateError.invalidResponse)) + return } - }) - } - - group.notify(queue: DispatchQueue.main) { - if let error = groupError { + + completion(.success((entryWrapper.entries))) + case .failure(let error): completion(.failure(error)) - } else { - completion(.success(groupEntries)) } - } + }) + case .failure(let error): completion(.failure(error)) From 0499952b45bc51c928abb59db929ef0ec9a089a2 Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Wed, 28 Oct 2020 19:03:41 -0500 Subject: [PATCH 2/8] Rewrote the article syncing code to be more efficient and handle large datasets --- .../ReaderAPI/ReaderAPIAccountDelegate.swift | 128 +----- .../Account/ReaderAPI/ReaderAPICaller.swift | 433 +++++------------- .../ReaderAPI/ReaderAPIUnreadEntry.swift | 2 +- 3 files changed, 151 insertions(+), 412 deletions(-) diff --git a/Account/Sources/Account/ReaderAPI/ReaderAPIAccountDelegate.swift b/Account/Sources/Account/ReaderAPI/ReaderAPIAccountDelegate.swift index 5b675736e..139837b4a 100644 --- a/Account/Sources/Account/ReaderAPI/ReaderAPIAccountDelegate.swift +++ b/Account/Sources/Account/ReaderAPI/ReaderAPIAccountDelegate.swift @@ -95,18 +95,25 @@ final class ReaderAPIAccountDelegate: AccountDelegate { refreshAccount(account) { result in switch result { case .success(): - self.sendArticleStatus(for: account) { _ in self.refreshProgress.completeTask() - self.refreshArticleStatus(for: account) { _ in + self.caller.retrieveItemIDs(type: .allForAccount) { result in self.refreshProgress.completeTask() - self.refreshArticles(account) { - self.refreshMissingArticles(account) { - self.refreshProgress.clear() - DispatchQueue.main.async { - completion(.success(())) + switch result { + case .success(let articleIDs): + account.markAsRead(Set(articleIDs)) { _ in + self.refreshArticleStatus(for: account) { _ in + self.refreshProgress.completeTask() + self.refreshMissingArticles(account) { + self.refreshProgress.clear() + DispatchQueue.main.async { + completion(.success(())) + } + } } } + case .failure(let error): + completion(.failure(error)) } } } @@ -173,10 +180,10 @@ final class ReaderAPIAccountDelegate: AccountDelegate { func refreshArticleStatus(for account: Account, completion: @escaping ((Result) -> Void)) { os_log(.debug, log: log, "Refreshing article statuses...") + let group = DispatchGroup() - group.enter() - caller.retrieveUnreadEntries() { result in + caller.retrieveItemIDs(type: .unread) { result in switch result { case .success(let articleIDs): self.syncArticleReadState(account: account, articleIDs: articleIDs) @@ -189,7 +196,7 @@ final class ReaderAPIAccountDelegate: AccountDelegate { } group.enter() - caller.retrieveStarredEntries() { result in + caller.retrieveItemIDs(type: .starred) { result in switch result { case .success(let articleIDs): self.syncArticleStarredState(account: account, articleIDs: articleIDs) @@ -403,7 +410,6 @@ final class ReaderAPIAccountDelegate: AccountDelegate { } func addWebFeed(for account: Account, with feed: WebFeed, to container: Container, completion: @escaping (Result) -> Void) { - if let folder = container as? Folder, let feedName = feed.externalID { refreshProgress.addToNumberOfTasksAndRemaining(1) caller.createTagging(subscriptionID: feedName, tagName: folder.name ?? "") { result in @@ -431,7 +437,6 @@ final class ReaderAPIAccountDelegate: AccountDelegate { completion(.success(())) } } - } func restoreWebFeed(for account: Account, feed: WebFeed, container: Container, completion: @escaping (Result) -> Void) { @@ -560,7 +565,6 @@ private extension ReaderAPIAccountDelegate { self.syncFolders(account, tags) } self.refreshProgress.completeTask() - self.forceExpireFolderFeedRelationship(account, tags) self.refreshFeeds(account, completion: completion) case .failure(let error): completion(.failure(error)) @@ -568,30 +572,6 @@ private extension ReaderAPIAccountDelegate { } } - func forceExpireFolderFeedRelationship(_ account: Account, _ tags: [ReaderAPITag]?) { - guard let tags = tags else { return } - - let folderNames: [String] = { - if let folders = account.folders { - return folders.map { $0.name ?? "" } - } else { - return [String]() - } - }() - - let readerFolderNames = tags.compactMap { $0.folderName } - - // The sync service has a tag that we don't have a folder for. We might not get a new - // taggings response for it if it is a folder rename. Force expire the subscription - // so that we will for sure get the new tagging information by pulling all subscriptions. - readerFolderNames.forEach { tagName in - if !folderNames.contains(tagName) { - accountMetadata?.conditionalGetInfo[ReaderAPICaller.ConditionalGetKeys.subscriptions] = nil - } - } - - } - func syncFolders(_ account: Account, _ tags: [ReaderAPITag]?) { guard let tags = tags else { return } assert(Thread.isMainThread) @@ -879,30 +859,21 @@ private extension ReaderAPIAccountDelegate { refreshProgress.addToNumberOfTasksAndRemaining(5) // Download the initial articles - self.caller.retrieveEntries(webFeedID: feed.webFeedID) { result in + self.caller.retrieveItemIDs(type: .allForFeed, webFeedID: feed.webFeedID) { result in self.refreshProgress.completeTask() - switch result { - case .success(let (entries, page)): - self.processEntries(account: account, entries: entries) { - + case .success(let articleIDs): + account.markAsRead(Set(articleIDs)) { _ in self.refreshProgress.completeTask() self.refreshArticleStatus(for: account) { _ in - self.refreshProgress.completeTask() - self.refreshArticles(account, page: page) { - - self.refreshProgress.completeTask() - self.refreshMissingArticles(account) { - - self.refreshProgress.clear() - DispatchQueue.main.async { - completion(.success(feed)) - } - + self.refreshMissingArticles(account) { + self.refreshProgress.clear() + DispatchQueue.main.async { + completion(.success(feed)) } - } + } } } @@ -914,29 +885,6 @@ private extension ReaderAPIAccountDelegate { } - func refreshArticles(_ account: Account, completion: @escaping (() -> Void)) { - os_log(.debug, log: log, "Refreshing articles...") - - caller.retrieveEntries() { result in - switch result { - case .success(let (entries, page, lastPageNumber)): - if let last = lastPageNumber { - self.refreshProgress.addToNumberOfTasksAndRemaining(last - 1) - } - self.processEntries(account: account, entries: entries) { - self.refreshProgress.completeTask() - self.refreshArticles(account, page: page) { - os_log(.debug, log: self.log, "Done refreshing articles.") - completion() - } - } - case .failure(let error): - os_log(.error, log: self.log, "Refresh articles failed: %@.", error.localizedDescription) - completion() - } - } - } - func refreshMissingArticles(_ account: Account, completion: @escaping VoidCompletionBlock) { account.fetchArticleIDsForStatusesWithoutArticlesNewerThanCutoffDate { articleIDsResult in @@ -981,32 +929,6 @@ private extension ReaderAPIAccountDelegate { } } - func refreshArticles(_ account: Account, page: String?, completion: @escaping (() -> Void)) { - - guard let page = page else { - completion() - return - } - - caller.retrieveEntries(page: page) { result in - - switch result { - case .success(let (entries, nextPage)): - - self.processEntries(account: account, entries: entries) { - self.refreshProgress.completeTask() - self.refreshArticles(account, page: nextPage, completion: completion) - } - - case .failure(let error): - os_log(.error, log: self.log, "Refresh articles for additional pages failed: %@.", error.localizedDescription) - completion() - } - - } - - } - func processEntries(account: Account, entries: [ReaderAPIEntry]?, completion: @escaping VoidCompletionBlock) { let parsedItems = mapEntriesToParsedItems(account: account, entries: entries) let webFeedIDsAndItems = Dictionary(grouping: parsedItems, by: { item in item.feedURL } ).mapValues { Set($0) } diff --git a/Account/Sources/Account/ReaderAPI/ReaderAPICaller.swift b/Account/Sources/Account/ReaderAPI/ReaderAPICaller.swift index a3e4fe06d..714403ebd 100644 --- a/Account/Sources/Account/ReaderAPI/ReaderAPICaller.swift +++ b/Account/Sources/Account/ReaderAPI/ReaderAPICaller.swift @@ -18,23 +18,23 @@ enum CreateReaderAPISubscriptionResult { final class ReaderAPICaller: NSObject { - struct ConditionalGetKeys { - static let subscriptions = "subscriptions" - static let tags = "tags" - static let unreadEntries = "unreadEntries" - static let starredEntries = "starredEntries" + enum ItemIDType { + case unread + case starred + case allForAccount + case allForFeed } - enum ReaderState: String { + private enum ReaderState: String { case read = "user/-/state/com.google/read" case starred = "user/-/state/com.google/starred" } - enum ReaderStreams: String { + private enum ReaderStreams: String { case readingList = "user/-/state/com.google/reading-list" } - enum ReaderAPIEndpoints: String { + private enum ReaderAPIEndpoints: String { case login = "/accounts/ClientLogin" case token = "/reader/api/0/token" case disableTag = "/reader/api/0/disable-tag" @@ -184,20 +184,16 @@ final class ReaderAPICaller: NSObject { return } - let conditionalGet = accountMetadata?.conditionalGetInfo[ConditionalGetKeys.tags] - var request = URLRequest(url: callURL, credentials: credentials, conditionalGet: conditionalGet) + var request = URLRequest(url: callURL, credentials: credentials) addVariantHeaders(&request) transport.send(request: request, resultType: ReaderAPITagContainer.self) { result in - switch result { - case .success(let (response, wrapper)): - self.storeConditionalGet(key: ConditionalGetKeys.tags, headers: response.allHeaderFields) + case .success(let (_, wrapper)): completion(.success(wrapper?.tags)) case .failure(let error): completion(.failure(error)) } - } } @@ -292,22 +288,17 @@ final class ReaderAPICaller: NSObject { return } - let conditionalGet = accountMetadata?.conditionalGetInfo[ConditionalGetKeys.subscriptions] - var request = URLRequest(url: callURL, credentials: credentials, conditionalGet: conditionalGet) + var request = URLRequest(url: callURL, credentials: credentials) addVariantHeaders(&request) transport.send(request: request, resultType: ReaderAPISubscriptionContainer.self) { result in - switch result { - case .success(let (response, container)): - self.storeConditionalGet(key: ConditionalGetKeys.subscriptions, headers: response.allHeaderFields) + case .success(let (_, container)): completion(.success(container?.subscriptions)) case .failure(let error): completion(.failure(error)) } - } - } func createSubscription(url: String, name: String?, folder: Folder?, completion: @escaping (Result) -> Void) { @@ -577,8 +568,14 @@ final class ReaderAPICaller: NSObject { request.httpMethod = "POST" // Get ids from above into hex representation of value - let idsToFetch = articleIDs.map({ (reference) -> String in - return "i=tag:google.com,2005:reader/item/\(reference)" + let idsToFetch = articleIDs.map({ articleID -> String in + if self.variant == .theOldReader { + return "i=tag:google.com,2005:reader/item/\(articleID)" + } else { + let idValue = Int(articleID)! + let idHexString = String(idValue, radix: 16, uppercase: false) + return "i=tag:google.com,2005:reader/item/\(idHexString)" + } }).joined(separator:"&") let postData = "T=\(token)&output=json&\(idsToFetch)".data(using: String.Encoding.utf8) @@ -604,254 +601,154 @@ final class ReaderAPICaller: NSObject { } } - - func retrieveEntries(webFeedID: String, completion: @escaping (Result<([ReaderAPIEntry]?, String?), Error>) -> Void) { - - let since = Calendar.current.date(byAdding: .month, value: -3, to: Date()) ?? Date() - + + func retrieveItemIDs(type: ItemIDType, webFeedID: String? = nil, completion: @escaping ((Result<[String], Error>) -> Void)) { guard let baseURL = APIBaseURL else { completion(.failure(CredentialsError.incompleteCredentials)) return } + var queryItems = [ + URLQueryItem(name: "n", value: "1000"), + URLQueryItem(name: "output", value: "json") + ] + + switch type { + case .allForAccount: + let since: Date = { + if let lastArticleFetch = self.accountMetadata?.lastArticleFetchStartTime { + return lastArticleFetch + } else { + return Calendar.current.date(byAdding: .month, value: -3, to: Date()) ?? Date() + } + }() + + let sinceTimeInterval = since.timeIntervalSince1970 + queryItems.append(URLQueryItem(name: "ot", value: String(Int(sinceTimeInterval)))) + queryItems.append(URLQueryItem(name: "s", value: ReaderStreams.readingList.rawValue)) + case .allForFeed: + guard let webFeedID = webFeedID else { + completion(.failure(ReaderAPIAccountDelegateError.invalidParameter)) + return + } + let sinceTimeInterval = (Calendar.current.date(byAdding: .month, value: -3, to: Date()) ?? Date()).timeIntervalSince1970 + queryItems.append(URLQueryItem(name: "ot", value: String(Int(sinceTimeInterval)))) + queryItems.append(URLQueryItem(name: "s", value: webFeedID)) + case .unread: + queryItems.append(URLQueryItem(name: "s", value: ReaderStreams.readingList.rawValue)) + queryItems.append(URLQueryItem(name: "xt", value: ReaderState.read.rawValue)) + case .starred: + queryItems.append(URLQueryItem(name: "s", value: ReaderState.starred.rawValue)) + } + let url = baseURL .appendingPathComponent(ReaderAPIEndpoints.itemIds.rawValue) - .appendingQueryItems([ - URLQueryItem(name: "s", value: webFeedID), - URLQueryItem(name: "ot", value: String(Int(since.timeIntervalSince1970))), - URLQueryItem(name: "output", value: "json") - ]) + .appendingQueryItems(queryItems) guard let callURL = url else { completion(.failure(TransportError.noURL)) return } - var request = URLRequest(url: callURL, credentials: credentials, conditionalGet: nil) - addVariantHeaders(&request) - - transport.send(request: request, resultType: ReaderAPIReferenceWrapper.self) { result in - - switch result { - case .success(let (_, unreadEntries)): - - guard let itemRefs = unreadEntries?.itemRefs else { - completion(.success(([], nil))) - return - } - - let itemIds = itemRefs.map { (reference) -> String in - if self.variant == .theOldReader { - return reference.itemId - } else { - // Convert the IDs to the (stupid) Google Hex Format - let idValue = Int(reference.itemId)! - return String(idValue, radix: 16, uppercase: false) - } - } - - self.retrieveEntries(articleIDs: itemIds) { (results) in - switch results { - case .success(let entries): - completion(.success((entries,nil))) - case .failure(let error): - completion(.failure(error)) - } - } - - case .failure(let error): - completion(.failure(error)) - } - - } - - } - - func retrieveEntries(completion: @escaping (Result<([ReaderAPIEntry]?, String?, Int?), Error>) -> Void) { - - guard let baseURL = APIBaseURL else { - completion(.failure(CredentialsError.incompleteCredentials)) - return - } - - let since: Date = { - if let lastArticleFetch = self.accountMetadata?.lastArticleFetchStartTime { - return lastArticleFetch - } else { - return Calendar.current.date(byAdding: .month, value: -3, to: Date()) ?? Date() - } - }() - - let sinceTimeInterval = since.timeIntervalSince1970 - let url = baseURL - .appendingPathComponent(ReaderAPIEndpoints.itemIds.rawValue) - .appendingQueryItems([ - URLQueryItem(name: "ot", value: String(Int(sinceTimeInterval))), - URLQueryItem(name: "n", value: "1000"), - URLQueryItem(name: "output", value: "json"), - URLQueryItem(name: "s", value: ReaderStreams.readingList.rawValue) - ]) - - guard let callURL = url else { - completion(.failure(TransportError.noURL)) - return - } - - var request = URLRequest(url: callURL, credentials: credentials) + var request: URLRequest = URLRequest(url: callURL, credentials: credentials) addVariantHeaders(&request) self.transport.send(request: request, resultType: ReaderAPIReferenceWrapper.self) { result in - switch result { case .success(let (response, entries)): - guard let entriesItemRefs = entries?.itemRefs, entriesItemRefs.count > 0 else { - completion(.success((nil, nil, nil))) + completion(.success([String]())) return } - - // This needs to be moved when we fix paging for item ids let dateInfo = HTTPDateInfo(urlResponse: response) + let itemIDs = entriesItemRefs.compactMap { $0.itemId } + self.retrieveItemIDs(type: type, url: callURL, dateInfo: dateInfo, itemIDs: itemIDs, continuation: entries?.continuation, completion: completion) + case .failure(let error): + completion(.failure(error)) + } + } + } + + func retrieveItemIDs(type: ItemIDType, url: URL, dateInfo: HTTPDateInfo?, itemIDs: [String], continuation: String?, completion: @escaping ((Result<[String], Error>) -> Void)) { + guard let continuation = continuation else { + if type == .allForAccount { self.accountMetadata?.lastArticleFetchStartTime = dateInfo?.date self.accountMetadata?.lastArticleFetchEndTime = Date() - - self.requestAuthorizationToken(endpoint: baseURL) { (result) in - switch result { - case .success(let token): - var request = URLRequest(url: baseURL.appendingPathComponent(ReaderAPIEndpoints.contents.rawValue), credentials: self.credentials) - self.addVariantHeaders(&request) - request.setValue("application/x-www-form-urlencoded", forHTTPHeaderField: "Content-Type") - request.httpMethod = "POST" - - let chunkedItemRefs = entriesItemRefs.chunked(into: 200) - let group = DispatchGroup() - var groupEntries = [ReaderAPIEntry]() - var groupError: Error? = nil - - for itemRefsChunk in chunkedItemRefs { - let itemFetchParameters = itemRefsChunk.map({ itemRef -> String in - if self.variant == .theOldReader { - return "i=tag:google.com,2005:reader/item/\(itemRef.itemId)" - } else { - let idValue = Int(itemRef.itemId)! - let idHexString = String(idValue, radix: 16, uppercase: false) - return "i=tag:google.com,2005:reader/item/\(idHexString)" - } - }).joined(separator:"&") - - let postData = "T=\(token)&output=json&\(itemFetchParameters)".data(using: String.Encoding.utf8) - - group.enter() - self.transport.send(request: request, method: HTTPMethod.post, data: postData!, resultType: ReaderAPIEntryWrapper.self, completion: { (result) in - switch result { - case .success(let (_, entryWrapper)): - guard let entryWrapper = entryWrapper else { - completion(.failure(ReaderAPIAccountDelegateError.invalidResponse)) - return - } - groupEntries.append(contentsOf: entryWrapper.entries) - group.leave() - case .failure(let error): - groupError = error - group.leave() - } - }) - } - - group.notify(queue: DispatchQueue.main) { - if let error = groupError { - completion(.failure(error)) - } else { - completion(.success((groupEntries, nil, nil))) - } - } - case .failure(let error): - completion(.failure(error)) - } - } - - case .failure(let error): - self.accountMetadata?.lastArticleFetchStartTime = nil - completion(.failure(error)) } - - } - } - - func retrieveEntries(page: String, completion: @escaping (Result<([ReaderAPIEntry]?, String?), Error>) -> Void) { - - guard let url = URL(string: page)?.appendingQueryItem(URLQueryItem(name: "mode", value: "extended")) else { - completion(.success((nil, nil))) - return - } - var request = URLRequest(url: url, credentials: credentials) - addVariantHeaders(&request) - - transport.send(request: request, resultType: [ReaderAPIEntry].self) { result in - - switch result { - case .success(let (response, entries)): - - let pagingInfo = HTTPLinkPagingInfo(urlResponse: response) - completion(.success((entries, pagingInfo.nextPage))) - - case .failure(let error): - self.accountMetadata?.lastArticleFetchStartTime = nil - completion(.failure(error)) - } - - } - - } - - func retrieveUnreadEntries(completion: @escaping (Result<[String]?, Error>) -> Void) { - - guard let baseURL = APIBaseURL else { - completion(.failure(CredentialsError.incompleteCredentials)) + completion(.success(itemIDs)) return } - let url = baseURL - .appendingPathComponent(ReaderAPIEndpoints.itemIds.rawValue) - .appendingQueryItems([ - URLQueryItem(name: "s", value: ReaderStreams.readingList.rawValue), - URLQueryItem(name: "n", value: "1000"), - URLQueryItem(name: "xt", value: ReaderState.read.rawValue), - URLQueryItem(name: "output", value: "json") - ]) + guard var urlComponents = URLComponents(url: url, resolvingAgainstBaseURL: false) else { + completion(.failure(ReaderAPIAccountDelegateError.invalidParameter)) + return + } - guard let callURL = url else { + var queryItems = urlComponents.queryItems!.filter({ $0.name != "c" }) + queryItems.append(URLQueryItem(name: "c", value: continuation)) + urlComponents.queryItems = queryItems + + guard let callURL = urlComponents.url else { completion(.failure(TransportError.noURL)) return } - - let conditionalGet = accountMetadata?.conditionalGetInfo[ConditionalGetKeys.unreadEntries] - var request = URLRequest(url: callURL, credentials: credentials, conditionalGet: conditionalGet) + + var request: URLRequest = URLRequest(url: callURL, credentials: credentials) addVariantHeaders(&request) - transport.send(request: request, resultType: ReaderAPIReferenceWrapper.self) { result in - + self.transport.send(request: request, resultType: ReaderAPIReferenceWrapper.self) { result in switch result { - case .success(let (response, unreadEntries)): - - guard let itemRefs = unreadEntries?.itemRefs else { - completion(.success([])) + case .success(let (_, entries)): + guard let entriesItemRefs = entries?.itemRefs, entriesItemRefs.count > 0 else { + self.retrieveItemIDs(type: type, url: callURL, dateInfo: dateInfo, itemIDs: itemIDs, continuation: entries?.continuation, completion: completion) return } - - let itemIds = itemRefs.map { $0.itemId } - - self.storeConditionalGet(key: ConditionalGetKeys.unreadEntries, headers: response.allHeaderFields) - completion(.success(itemIds)) + var totalItemIDs = itemIDs + totalItemIDs.append(contentsOf: entriesItemRefs.compactMap { $0.itemId }) + self.retrieveItemIDs(type: type, url: callURL, dateInfo: dateInfo, itemIDs: totalItemIDs, continuation: entries?.continuation, completion: completion) case .failure(let error): completion(.failure(error)) } - } - } - func updateStateToEntries(entries: [String], state: ReaderState, add: Bool, completion: @escaping (Result) -> Void) { + func createUnreadEntries(entries: [String], completion: @escaping (Result) -> Void) { + updateStateToEntries(entries: entries, state: .read, add: false, completion: completion) + } + + func deleteUnreadEntries(entries: [String], completion: @escaping (Result) -> Void) { + updateStateToEntries(entries: entries, state: .read, add: true, completion: completion) + } + + func createStarredEntries(entries: [String], completion: @escaping (Result) -> Void) { + updateStateToEntries(entries: entries, state: .starred, add: true, completion: completion) + } + + func deleteStarredEntries(entries: [String], completion: @escaping (Result) -> Void) { + updateStateToEntries(entries: entries, state: .starred, add: false, completion: completion) + } + +} + +// MARK: Private + +private extension ReaderAPICaller { + + func storeConditionalGet(key: String, headers: [AnyHashable : Any]) { + if var conditionalGet = accountMetadata?.conditionalGetInfo { + conditionalGet[key] = HTTPConditionalGetInfo(headers: headers) + accountMetadata?.conditionalGetInfo = conditionalGet + } + } + + func addVariantHeaders(_ request: inout URLRequest) { + if variant == .inoreader { + request.addValue(SecretsManager.provider.inoreaderAppId, forHTTPHeaderField: "AppId") + request.addValue(SecretsManager.provider.inoreaderAppKey, forHTTPHeaderField: "AppKey") + } + } + + private func updateStateToEntries(entries: [String], state: ReaderState, add: Bool, completion: @escaping (Result) -> Void) { guard let baseURL = APIBaseURL else { completion(.failure(CredentialsError.incompleteCredentials)) return @@ -897,85 +794,5 @@ final class ReaderAPICaller: NSObject { } } - func createUnreadEntries(entries: [String], completion: @escaping (Result) -> Void) { - updateStateToEntries(entries: entries, state: .read, add: false, completion: completion) - } - - func deleteUnreadEntries(entries: [String], completion: @escaping (Result) -> Void) { - updateStateToEntries(entries: entries, state: .read, add: true, completion: completion) - } - - func createStarredEntries(entries: [String], completion: @escaping (Result) -> Void) { - updateStateToEntries(entries: entries, state: .starred, add: true, completion: completion) - - } - - func deleteStarredEntries(entries: [String], completion: @escaping (Result) -> Void) { - updateStateToEntries(entries: entries, state: .starred, add: false, completion: completion) - } - - func retrieveStarredEntries(completion: @escaping (Result<[String]?, Error>) -> Void) { - guard let baseURL = APIBaseURL else { - completion(.failure(CredentialsError.incompleteCredentials)) - return - } - - let url = baseURL - .appendingPathComponent(ReaderAPIEndpoints.itemIds.rawValue) - .appendingQueryItems([ - URLQueryItem(name: "s", value: ReaderState.starred.rawValue), - URLQueryItem(name: "n", value: "1000"), - URLQueryItem(name: "output", value: "json") - ]) - - guard let callURL = url else { - completion(.failure(TransportError.noURL)) - return - } - - let conditionalGet = accountMetadata?.conditionalGetInfo[ConditionalGetKeys.starredEntries] - var request = URLRequest(url: callURL, credentials: credentials, conditionalGet: conditionalGet) - addVariantHeaders(&request) - - transport.send(request: request, resultType: ReaderAPIReferenceWrapper.self) { result in - - switch result { - case .success(let (response, unreadEntries)): - guard let itemRefs = unreadEntries?.itemRefs else { - completion(.success([])) - return - } - - let itemIds = itemRefs.map { $0.itemId } - self.storeConditionalGet(key: ConditionalGetKeys.starredEntries, headers: response.allHeaderFields) - completion(.success(itemIds)) - case .failure(let error): - completion(.failure(error)) - } - - } - - } - -} - -// MARK: Private - -private extension ReaderAPICaller { - - func storeConditionalGet(key: String, headers: [AnyHashable : Any]) { - if var conditionalGet = accountMetadata?.conditionalGetInfo { - conditionalGet[key] = HTTPConditionalGetInfo(headers: headers) - accountMetadata?.conditionalGetInfo = conditionalGet - } - } - - func addVariantHeaders(_ request: inout URLRequest) { - if variant == .inoreader { - request.addValue(SecretsManager.provider.inoreaderAppId, forHTTPHeaderField: "AppId") - request.addValue(SecretsManager.provider.inoreaderAppKey, forHTTPHeaderField: "AppKey") - } - } - } diff --git a/Account/Sources/Account/ReaderAPI/ReaderAPIUnreadEntry.swift b/Account/Sources/Account/ReaderAPI/ReaderAPIUnreadEntry.swift index 147310f34..4fe3fde89 100644 --- a/Account/Sources/Account/ReaderAPI/ReaderAPIUnreadEntry.swift +++ b/Account/Sources/Account/ReaderAPI/ReaderAPIUnreadEntry.swift @@ -19,7 +19,7 @@ struct ReaderAPIReferenceWrapper: Codable { } struct ReaderAPIReference: Codable { - let itemId: String + let itemId: String? enum CodingKeys: String, CodingKey { case itemId = "id" From a604003c893877d770e3e42745b44b17f1b0a8cb Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Wed, 28 Oct 2020 19:26:16 -0500 Subject: [PATCH 3/8] Remove dead code to fix compiler warning. --- Shared/AccountType+Helpers.swift | 6 ------ 1 file changed, 6 deletions(-) diff --git a/Shared/AccountType+Helpers.swift b/Shared/AccountType+Helpers.swift index fc1a5d221..d83c8fa28 100644 --- a/Shared/AccountType+Helpers.swift +++ b/Shared/AccountType+Helpers.swift @@ -51,14 +51,8 @@ extension AccountType { return NSLocalizedString("NewsBlur", comment: "Account name") case .theOldReader: return NSLocalizedString("The Old Reader", comment: "Account name") - default: - return "" } - - - - } } From 3a1570f6c6d0d6652fe0714efe2a266f65ab8308 Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Thu, 29 Oct 2020 13:52:23 -0500 Subject: [PATCH 4/8] Fix feed wrangler error message layout --- .../Accounts/AccountsFeedWrangler.xib | 42 ++++++++++--------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/Mac/Preferences/Accounts/AccountsFeedWrangler.xib b/Mac/Preferences/Accounts/AccountsFeedWrangler.xib index 684cdb225..308836ac0 100644 --- a/Mac/Preferences/Accounts/AccountsFeedWrangler.xib +++ b/Mac/Preferences/Accounts/AccountsFeedWrangler.xib @@ -1,11 +1,12 @@ - + - + + - + @@ -21,13 +22,13 @@ - + - + @@ -38,7 +39,7 @@ - + @@ -56,7 +57,10 @@ - + + + + @@ -69,7 +73,7 @@ - + @@ -79,7 +83,7 @@ - + @@ -92,7 +96,7 @@ - + @@ -102,7 +106,7 @@ - + @@ -116,21 +120,21 @@ - + + - - + + - @@ -180,6 +184,6 @@ Gw - + From 515dd5f63f8dca220be5540c98aab3ee5131e5fe Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Thu, 29 Oct 2020 14:05:55 -0500 Subject: [PATCH 5/8] Check for duplicate accounts and make sure it doesn't interfere with credentials updates --- .../AccountsFeedWranglerWindowController.swift | 2 +- .../AccountsNewsBlurWindowController.swift | 2 +- .../AccountsReaderAPIWindowController.swift | 2 +- .../FeedWranglerAccountViewController.swift | 2 +- iOS/Account/NewsBlurAccountViewController.swift | 10 ++++++++-- iOS/Account/ReaderAPIAccountViewController.swift | 15 +++++++++------ 6 files changed, 21 insertions(+), 12 deletions(-) diff --git a/Mac/Preferences/Accounts/AccountsFeedWranglerWindowController.swift b/Mac/Preferences/Accounts/AccountsFeedWranglerWindowController.swift index 5f2e2081b..5b84ce7af 100644 --- a/Mac/Preferences/Accounts/AccountsFeedWranglerWindowController.swift +++ b/Mac/Preferences/Accounts/AccountsFeedWranglerWindowController.swift @@ -56,7 +56,7 @@ class AccountsFeedWranglerWindowController: NSWindowController { return } - guard !AccountManager.shared.duplicateServiceAccount(type: .feedWrangler, username: usernameTextField.stringValue) else { + guard account != nil || !AccountManager.shared.duplicateServiceAccount(type: .feedWrangler, username: usernameTextField.stringValue) else { self.errorMessageLabel.stringValue = NSLocalizedString("There is already a FeedWrangler account with that username created.", comment: "Duplicate Error") return } diff --git a/Mac/Preferences/Accounts/AccountsNewsBlurWindowController.swift b/Mac/Preferences/Accounts/AccountsNewsBlurWindowController.swift index 723e254d1..e3fc527b2 100644 --- a/Mac/Preferences/Accounts/AccountsNewsBlurWindowController.swift +++ b/Mac/Preferences/Accounts/AccountsNewsBlurWindowController.swift @@ -56,7 +56,7 @@ class AccountsNewsBlurWindowController: NSWindowController { return } - guard !AccountManager.shared.duplicateServiceAccount(type: .newsBlur, username: usernameTextField.stringValue) else { + guard account != nil || !AccountManager.shared.duplicateServiceAccount(type: .newsBlur, username: usernameTextField.stringValue) else { self.errorMessageLabel.stringValue = NSLocalizedString("There is already a NewsBlur account with that username created.", comment: "Duplicate Error") return } diff --git a/Mac/Preferences/Accounts/AccountsReaderAPIWindowController.swift b/Mac/Preferences/Accounts/AccountsReaderAPIWindowController.swift index 66a286151..ccec1dedd 100644 --- a/Mac/Preferences/Accounts/AccountsReaderAPIWindowController.swift +++ b/Mac/Preferences/Accounts/AccountsReaderAPIWindowController.swift @@ -91,7 +91,7 @@ class AccountsReaderAPIWindowController: NSWindowController { return } - guard !AccountManager.shared.duplicateServiceAccount(type: accountType, username: usernameTextField.stringValue) else { + guard account != nil || !AccountManager.shared.duplicateServiceAccount(type: accountType, username: usernameTextField.stringValue) else { self.errorMessageLabel.stringValue = NSLocalizedString("There is already an account of this type with that username created.", comment: "Duplicate Error") return } diff --git a/iOS/Account/FeedWranglerAccountViewController.swift b/iOS/Account/FeedWranglerAccountViewController.swift index 06bf376ee..c5dd3944c 100644 --- a/iOS/Account/FeedWranglerAccountViewController.swift +++ b/iOS/Account/FeedWranglerAccountViewController.swift @@ -80,7 +80,7 @@ class FeedWranglerAccountViewController: UITableViewController { // When you fill in the email address via auto-complete it adds extra whitespace let trimmedEmail = email.trimmingCharacters(in: .whitespaces) - guard !AccountManager.shared.duplicateServiceAccount(type: .feedWrangler, username: trimmedEmail) else { + guard account != nil || !AccountManager.shared.duplicateServiceAccount(type: .feedWrangler, username: trimmedEmail) else { showError(NSLocalizedString("There is already a FeedWrangler account with that username created.", comment: "Duplicate Error")) return } diff --git a/iOS/Account/NewsBlurAccountViewController.swift b/iOS/Account/NewsBlurAccountViewController.swift index c1bed5e2d..cf942c3f4 100644 --- a/iOS/Account/NewsBlurAccountViewController.swift +++ b/iOS/Account/NewsBlurAccountViewController.swift @@ -80,13 +80,19 @@ class NewsBlurAccountViewController: UITableViewController { return } + // When you fill in the email address via auto-complete it adds extra whitespace + let trimmedUsername = username.trimmingCharacters(in: .whitespaces) + + guard account != nil || !AccountManager.shared.duplicateServiceAccount(type: .newsBlur, username: trimmedUsername) else { + showError(NSLocalizedString("There is already a NewsBlur account with that username created.", comment: "Duplicate Error")) + return + } + let password = passwordTextField.text ?? "" startAnimatingActivityIndicator() disableNavigation() - // When you fill in the email address via auto-complete it adds extra whitespace - let trimmedUsername = username.trimmingCharacters(in: .whitespaces) let credentials = Credentials(type: .newsBlurBasic, username: trimmedUsername, secret: password) Account.validateCredentials(type: .newsBlur, credentials: credentials) { result in diff --git a/iOS/Account/ReaderAPIAccountViewController.swift b/iOS/Account/ReaderAPIAccountViewController.swift index 676dcb1ad..3eca028cd 100644 --- a/iOS/Account/ReaderAPIAccountViewController.swift +++ b/iOS/Account/ReaderAPIAccountViewController.swift @@ -109,7 +109,7 @@ class ReaderAPIAccountViewController: UITableViewController { } @IBAction func action(_ sender: Any) { - guard validateDataEntry() else { + guard validateDataEntry(), let type = accountType else { return } @@ -117,15 +117,18 @@ class ReaderAPIAccountViewController: UITableViewController { let password = passwordTextField.text! let url = apiURL()! + // When you fill in the email address via auto-complete it adds extra whitespace + let trimmedUsername = username.trimmingCharacters(in: .whitespaces) + + guard account != nil || !AccountManager.shared.duplicateServiceAccount(type: type, username: trimmedUsername) else { + showError(NSLocalizedString("There is already an account of that type with that username created.", comment: "Duplicate Error")) + return + } + startAnimatingActivityIndicator() disableNavigation() - // When you fill in the email address via auto-complete it adds extra whitespace - let trimmedUsername = username.trimmingCharacters(in: .whitespaces) let credentials = Credentials(type: .readerBasic, username: trimmedUsername, secret: password) - guard let type = accountType else { - return - } Account.validateCredentials(type: type, credentials: credentials, endpoint: url) { result in self.stopAnimatingActivityIndicator() From 1a4ab96038455f5fd588c8bf7e6f080249653960 Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Thu, 29 Oct 2020 14:53:52 -0500 Subject: [PATCH 6/8] Enable credentials button for newer sync services --- .../ReaderAPIAccountViewController.swift | 1 - .../AccountInspectorViewController.swift | 19 +++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/iOS/Account/ReaderAPIAccountViewController.swift b/iOS/Account/ReaderAPIAccountViewController.swift index 3eca028cd..5c99618d6 100644 --- a/iOS/Account/ReaderAPIAccountViewController.swift +++ b/iOS/Account/ReaderAPIAccountViewController.swift @@ -21,7 +21,6 @@ class ReaderAPIAccountViewController: UITableViewController { @IBOutlet weak var showHideButton: UIButton! @IBOutlet weak var actionButton: UIButton! - weak var account: Account? var accountType: AccountType? weak var delegate: AddAccountDismissDelegate? diff --git a/iOS/Inspector/AccountInspectorViewController.swift b/iOS/Inspector/AccountInspectorViewController.swift index cc7c5be5b..638383f62 100644 --- a/iOS/Inspector/AccountInspectorViewController.swift +++ b/iOS/Inspector/AccountInspectorViewController.swift @@ -58,6 +58,25 @@ class AccountInspectorViewController: UITableViewController { addViewController.account = account navController.modalPresentationStyle = .currentContext present(navController, animated: true) + case .feedWrangler: + let navController = UIStoryboard.account.instantiateViewController(withIdentifier: "FeedWranglerAccountNavigationViewController") as! UINavigationController + let addViewController = navController.topViewController as! FeedWranglerAccountViewController + addViewController.account = account + navController.modalPresentationStyle = .currentContext + present(navController, animated: true) + case .newsBlur: + let navController = UIStoryboard.account.instantiateViewController(withIdentifier: "NewsBlurAccountNavigationViewController") as! UINavigationController + let addViewController = navController.topViewController as! NewsBlurAccountViewController + addViewController.account = account + navController.modalPresentationStyle = .currentContext + present(navController, animated: true) + case .inoreader, .bazQux, .theOldReader, .freshRSS: + let navController = UIStoryboard.account.instantiateViewController(withIdentifier: "ReaderAPIAccountNavigationViewController") as! UINavigationController + let addViewController = navController.topViewController as! ReaderAPIAccountViewController + addViewController.accountType = account.type + addViewController.account = account + navController.modalPresentationStyle = .currentContext + present(navController, animated: true) default: break } From 1cfe248c6a130d614c94126f4584122594945825 Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Thu, 29 Oct 2020 15:06:27 -0500 Subject: [PATCH 7/8] Store the NewsBlur basic credentials so that they can be updated --- iOS/Account/NewsBlurAccountViewController.swift | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/iOS/Account/NewsBlurAccountViewController.swift b/iOS/Account/NewsBlurAccountViewController.swift index cf942c3f4..6d5dab361 100644 --- a/iOS/Account/NewsBlurAccountViewController.swift +++ b/iOS/Account/NewsBlurAccountViewController.swift @@ -93,15 +93,15 @@ class NewsBlurAccountViewController: UITableViewController { startAnimatingActivityIndicator() disableNavigation() - let credentials = Credentials(type: .newsBlurBasic, username: trimmedUsername, secret: password) - Account.validateCredentials(type: .newsBlur, credentials: credentials) { result in + let basicCredentials = Credentials(type: .newsBlurBasic, username: trimmedUsername, secret: password) + Account.validateCredentials(type: .newsBlur, credentials: basicCredentials) { result in self.stopAnimatingActivityIndicator() self.enableNavigation() switch result { - case .success(let credentials): - if let credentials = credentials { + case .success(let sessionCredentials): + if let sessionCredentials = sessionCredentials { var newAccount = false if self.account == nil { self.account = AccountManager.shared.createAccount(type: .newsBlur) @@ -112,8 +112,10 @@ class NewsBlurAccountViewController: UITableViewController { do { try self.account?.removeCredentials(type: .newsBlurBasic) + try self.account?.removeCredentials(type: .newsBlurSessionId) } catch {} - try self.account?.storeCredentials(credentials) + try self.account?.storeCredentials(basicCredentials) + try self.account?.storeCredentials(sessionCredentials) if newAccount { self.account?.refreshAll() { result in From 64c93ed118ba621d73b1b96c0dbdcf943e71fee0 Mon Sep 17 00:00:00 2001 From: Maurice Parker Date: Thu, 29 Oct 2020 15:19:30 -0500 Subject: [PATCH 8/8] Enable credentials for newer sync services --- .../Accounts/AccountsDetailViewController.swift | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/Mac/Preferences/Accounts/AccountsDetailViewController.swift b/Mac/Preferences/Accounts/AccountsDetailViewController.swift index 5ca513c02..390fb3c18 100644 --- a/Mac/Preferences/Accounts/AccountsDetailViewController.swift +++ b/Mac/Preferences/Accounts/AccountsDetailViewController.swift @@ -72,18 +72,23 @@ final class AccountsDetailViewController: NSViewController, NSTextFieldDelegate accountsFeedbinWindowController.account = account accountsFeedbinWindowController.runSheetOnWindow(self.view.window!) accountsWindowController = accountsFeedbinWindowController - case .freshRSS: - let accountsFreshRSSWindowController = AccountsReaderAPIWindowController() - accountsFreshRSSWindowController.accountType = account.type - accountsFreshRSSWindowController.account = account - accountsFreshRSSWindowController.runSheetOnWindow(self.view.window!) - accountsWindowController = accountsFreshRSSWindowController + case .inoreader, .bazQux, .theOldReader, .freshRSS: + let accountsReaderAPIWindowController = AccountsReaderAPIWindowController() + accountsReaderAPIWindowController.accountType = account.type + accountsReaderAPIWindowController.account = account + accountsReaderAPIWindowController.runSheetOnWindow(self.view.window!) + accountsWindowController = accountsReaderAPIWindowController break case .feedWrangler: let accountsFeedWranglerWindowController = AccountsFeedWranglerWindowController() accountsFeedWranglerWindowController.account = account accountsFeedWranglerWindowController.runSheetOnWindow(self.view.window!) accountsWindowController = accountsFeedWranglerWindowController + case .newsBlur: + let accountsNewsBlurWindowController = AccountsNewsBlurWindowController() + accountsNewsBlurWindowController.account = account + accountsNewsBlurWindowController.runSheetOnWindow(self.view.window!) + accountsWindowController = accountsNewsBlurWindowController default: break }