From e5a2c7057d0696f8fd3ce7a37985e73adccdc846 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrien=20Coye=20de=20Brune=CC=81lis?= Date: Tue, 27 Feb 2024 17:08:46 +0100 Subject: [PATCH 1/8] fix: Make sure no PausedNotification is produced by the FileProvider --- .../Data/UploadQueue/Queue/UploadQueue+Notifications.swift | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/kDriveCore/Data/UploadQueue/Queue/UploadQueue+Notifications.swift b/kDriveCore/Data/UploadQueue/Queue/UploadQueue+Notifications.swift index 51451cbed..3ccc113c7 100644 --- a/kDriveCore/Data/UploadQueue/Queue/UploadQueue+Notifications.swift +++ b/kDriveCore/Data/UploadQueue/Queue/UploadQueue+Notifications.swift @@ -42,6 +42,11 @@ extension UploadQueue: UploadNotifiable { public func sendPausedNotificationIfNeeded() { Log.uploadQueue("sendPausedNotificationIfNeeded") + guard appContextService.context != .fileProviderExtension else { + Log.uploadQueue("\(#function) disabled in FileProviderExtension", level: .error) + return + } + serialQueue.async { [weak self] in guard let self else { return } if !pausedNotificationSent { From 02dcfb716b5d095c008ac42333ad1191e5fea51b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrien=20Coye=20de=20Brune=CC=81lis?= Date: Tue, 27 Feb 2024 19:24:10 +0100 Subject: [PATCH 2/8] fix: Rewrote algorithm to prevent a potential race condition --- kDriveCore/Data/Cache/DriveInfosManager.swift | 79 +++++++++++-------- 1 file changed, 45 insertions(+), 34 deletions(-) diff --git a/kDriveCore/Data/Cache/DriveInfosManager.swift b/kDriveCore/Data/Cache/DriveInfosManager.swift index e55a32249..2d77ed660 100644 --- a/kDriveCore/Data/Cache/DriveInfosManager.swift +++ b/kDriveCore/Data/Cache/DriveInfosManager.swift @@ -19,6 +19,7 @@ import CocoaLumberjackSwift import FileProvider import Foundation +import InfomaniakConcurrency import InfomaniakCore import Realm import RealmSwift @@ -116,6 +117,8 @@ public class DriveInfosManager { drive.sharedWithMe = sharedWithMe } + private typealias FilteredDomain = (new: NSFileProviderDomain, existing: NSFileProviderDomain?) + private func initFileProviderDomains(drives: [Drive], user: InfomaniakCore.UserProfile) { // Clean file provider storage if needed if UserDefaults.shared.fpStorageVersion < currentFpStorageVersion { @@ -129,54 +132,62 @@ public class DriveInfosManager { } UserDefaults.shared.fpStorageVersion = currentFpStorageVersion } catch { - // Silently handle error + // TODO: Sentry } } - let updatedDomains = drives.map { - NSFileProviderDomain( - identifier: NSFileProviderDomainIdentifier($0.objectId), - displayName: "\($0.name) (\(user.email))", - pathRelativeToDocumentStorage: "\($0.objectId)" - ) - } + // TODO: Start Activity Task { + let updatedDomains = drives.map { + NSFileProviderDomain( + identifier: NSFileProviderDomainIdentifier($0.objectId), + displayName: "\($0.name) (\(user.email))", + pathRelativeToDocumentStorage: "\($0.objectId)" + ) + } + do { let allDomains = try await NSFileProviderManager.domains() - var domainsForCurrentUser = allDomains.filter { $0.identifier.rawValue.hasSuffix("_\(user.id)") } - await withThrowingTaskGroup(of: Void.self) { group in - for newDomain in updatedDomains { - // Check if domain already added - if let existingDomainIndex = domainsForCurrentUser - .firstIndex(where: { $0.identifier == newDomain.identifier }) { - let existingDomain = domainsForCurrentUser.remove(at: existingDomainIndex) - // Domain exists but its name could have changed - if existingDomain.displayName != newDomain.displayName { - group.addTask { - try await NSFileProviderManager.remove(existingDomain) - try await NSFileProviderManager.add(newDomain) - } - } - } else { - // Domain didn't exist we have to add it - group.addTask { - try await NSFileProviderManager.add(newDomain) - } - } + let existingDomainsForCurrentUser = allDomains.filter { $0.identifier.rawValue.hasSuffix("_\(user.id)") } + + let updatedDomainsForCurrentUser: [FilteredDomain] = updatedDomains.map { newDomain in + let existingDomain = existingDomainsForCurrentUser.first { $0.identifier == newDomain.identifier } + return (newDomain, existingDomain) + } + + try await updatedDomainsForCurrentUser.concurrentForEach(customConcurrency: 1) { domain in + // Simply add domain if new + let newDomain = domain.new + guard let existingDomain = domain.existing else { + try await NSFileProviderManager.add(newDomain) + return + } + + // Update existing accounts if necessary + if existingDomain.displayName != newDomain.displayName { + try await NSFileProviderManager.remove(existingDomain) + try await NSFileProviderManager.add(newDomain) } } - // Remove left domains - await withThrowingTaskGroup(of: Void.self) { group in - for domain in domainsForCurrentUser { - group.addTask { - try await NSFileProviderManager.remove(domain) - } + // Remove domains no longer present for current user + let removedDomainsForCurrentUser = updatedDomains.filter { updatedDomain in + guard existingDomainsForCurrentUser.contains(where: { $0.identifier == updatedDomain.identifier }) else { + return true } + + return false + } + + try await removedDomainsForCurrentUser.concurrentForEach(customConcurrency: 1) { oldDomain in + try await NSFileProviderManager.remove(oldDomain) } } catch { DDLogError("Error while updating file provider domains: \(error)") + // TODO: add Sentry } + + // TODO: notify for consistency } } From ef191a9dce37c87193d27af0ed312639e03c730c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrien=20Coye=20de=20Brune=CC=81lis?= Date: Wed, 28 Feb 2024 10:33:55 +0100 Subject: [PATCH 3/8] feat: Signal Files.app on FileProviderDomain change refactor: Split DriveInfoManager + FileProvider --- .../DriveInfosManager+FileProvider.swift | 174 ++++++++++++++++++ .../DriveInfosManager.swift | 137 +------------- .../FileProviderDomain+Identifier.swift | 45 +++++ 3 files changed, 224 insertions(+), 132 deletions(-) create mode 100644 kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift rename kDriveCore/Data/Cache/{ => DriveInfosManager}/DriveInfosManager.swift (61%) create mode 100644 kDriveCore/Utils/FileProvider/FileProviderDomain+Identifier.swift diff --git a/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift b/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift new file mode 100644 index 000000000..64024631d --- /dev/null +++ b/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift @@ -0,0 +1,174 @@ +/* + Infomaniak kDrive - iOS App + Copyright (C) 2024 Infomaniak Network SA + + This program is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . + */ + +import CocoaLumberjackSwift +import FileProvider +import Foundation +import InfomaniakConcurrency +import InfomaniakCore + +public extension DriveInfosManager { + private typealias FilteredDomain = (new: NSFileProviderDomain, existing: NSFileProviderDomain?) + + internal func initFileProviderDomains(drives: [Drive], user: InfomaniakCore.UserProfile) { + // Clean file provider storage if needed + if UserDefaults.shared.fpStorageVersion < currentFpStorageVersion { + do { + let fileURLs = try FileManager.default.contentsOfDirectory( + at: NSFileProviderManager.default.documentStorageURL, + includingPropertiesForKeys: nil + ) + for url in fileURLs { + try FileManager.default.removeItem(at: url) + } + UserDefaults.shared.fpStorageVersion = currentFpStorageVersion + } catch { + // TODO: Sentry + } + } + + // TODO: Start Activity + Task { + let updatedDomains = drives.map { + NSFileProviderDomain( + identifier: NSFileProviderDomainIdentifier($0.objectId), + displayName: "\($0.name) (\(user.email))", + pathRelativeToDocumentStorage: "\($0.objectId)" + ) + } + + do { + let allDomains = try await NSFileProviderManager.domains() + let existingDomainsForCurrentUser = allDomains.filter { $0.identifier.rawValue.hasSuffix("_\(user.id)") } + + let updatedDomainsForCurrentUser: [FilteredDomain] = updatedDomains.map { newDomain in + let existingDomain = existingDomainsForCurrentUser.first { $0.identifier == newDomain.identifier } + return (newDomain, existingDomain) + } + + try await updatedDomainsForCurrentUser.concurrentForEach(customConcurrency: 1) { domain in + // Simply add domain if new + let newDomain = domain.new + guard let existingDomain = domain.existing else { + try await NSFileProviderManager.add(newDomain) + return + } + + // Update existing accounts if necessary + if existingDomain.displayName != newDomain.displayName { + try await NSFileProviderManager.remove(existingDomain) + try await NSFileProviderManager.add(newDomain) + self.signalChanges(for: newDomain) + } + } + + // Remove domains no longer present for current user + let removedDomainsForCurrentUser = updatedDomains.filter { updatedDomain in + guard existingDomainsForCurrentUser.contains(where: { $0.identifier == updatedDomain.identifier }) else { + return true + } + + return false + } + + try await removedDomainsForCurrentUser.concurrentForEach(customConcurrency: 1) { oldDomain in + try await NSFileProviderManager.remove(oldDomain) + } + } catch { + DDLogError("Error while updating file provider domains: \(error)") + // TODO: add Sentry + } + + // TODO: notify for consistency + } + } + + internal func deleteFileProviderDomains(for userId: Int) { + NSFileProviderManager.getDomainsWithCompletionHandler { allDomains, error in + if let error { + DDLogError("Error while getting domains: \(error)") + } + + let domainsForCurrentUser = allDomains.filter { $0.identifier.rawValue.hasSuffix("_\(userId)") } + for domain in domainsForCurrentUser { + NSFileProviderManager.remove(domain) { error in + if let error { + DDLogError("Error while removing domain \(domain.displayName): \(error)") + } + } + } + } + } + + func deleteAllFileProviderDomains() { + NSFileProviderManager.removeAllDomains { error in + if let error { + DDLogError("Error while removing domains: \(error)") + } + } + } + + internal func getFileProviderDomain(for driveId: String, completion: @escaping (NSFileProviderDomain?) -> Void) { + NSFileProviderManager.getDomainsWithCompletionHandler { domains, error in + if let error { + DDLogError("Error while getting domains: \(error)") + completion(nil) + } else { + completion(domains.first { $0.identifier.rawValue == driveId }) + } + } + } + + func getFileProviderManager(for drive: Drive, completion: @escaping (NSFileProviderManager) -> Void) { + getFileProviderManager(for: drive.objectId, completion: completion) + } + + func getFileProviderManager(driveId: Int, userId: Int, completion: @escaping (NSFileProviderManager) -> Void) { + let objectId = DriveInfosManager.getObjectId(driveId: driveId, userId: userId) + getFileProviderManager(for: objectId, completion: completion) + } + + func getFileProviderManager(for driveId: String, completion: @escaping (NSFileProviderManager) -> Void) { + getFileProviderDomain(for: driveId) { domain in + if let domain { + completion(NSFileProviderManager(for: domain) ?? .default) + } else { + completion(.default) + } + } + } + + // MARK: Signal + + /// Signal changes on this Drive to the File Provider Extension + private func signalChanges(for domain: NSFileProviderDomain) { + guard let driveId = domain.driveId, let userId = domain.userId else { + // Sentry + return + } + + DriveInfosManager.instance.getFileProviderManager(driveId: driveId, userId: userId) { manager in + manager.signalEnumerator(for: .workingSet) { _ in + // META: keep SonarCloud happy + } + manager.signalEnumerator(for: .rootContainer) { _ in + // META: keep SonarCloud happy + } + } + } +} diff --git a/kDriveCore/Data/Cache/DriveInfosManager.swift b/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager.swift similarity index 61% rename from kDriveCore/Data/Cache/DriveInfosManager.swift rename to kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager.swift index 2d77ed660..d60819b3f 100644 --- a/kDriveCore/Data/Cache/DriveInfosManager.swift +++ b/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager.swift @@ -19,16 +19,18 @@ import CocoaLumberjackSwift import FileProvider import Foundation -import InfomaniakConcurrency import InfomaniakCore import Realm import RealmSwift import Sentry -public class DriveInfosManager { +public final class DriveInfosManager { + // TODO: use DI public static let instance = DriveInfosManager() private static let currentDbVersion: UInt64 = 9 - private let currentFpStorageVersion = 1 + + let currentFpStorageVersion = 1 + public let realmConfiguration: Realm.Configuration private let dbName = "DrivesInfos.realm" private var fileProviderManagers: [String: NSFileProviderManager] = [:] @@ -117,135 +119,6 @@ public class DriveInfosManager { drive.sharedWithMe = sharedWithMe } - private typealias FilteredDomain = (new: NSFileProviderDomain, existing: NSFileProviderDomain?) - - private func initFileProviderDomains(drives: [Drive], user: InfomaniakCore.UserProfile) { - // Clean file provider storage if needed - if UserDefaults.shared.fpStorageVersion < currentFpStorageVersion { - do { - let fileURLs = try FileManager.default.contentsOfDirectory( - at: NSFileProviderManager.default.documentStorageURL, - includingPropertiesForKeys: nil - ) - for url in fileURLs { - try FileManager.default.removeItem(at: url) - } - UserDefaults.shared.fpStorageVersion = currentFpStorageVersion - } catch { - // TODO: Sentry - } - } - - // TODO: Start Activity - Task { - let updatedDomains = drives.map { - NSFileProviderDomain( - identifier: NSFileProviderDomainIdentifier($0.objectId), - displayName: "\($0.name) (\(user.email))", - pathRelativeToDocumentStorage: "\($0.objectId)" - ) - } - - do { - let allDomains = try await NSFileProviderManager.domains() - let existingDomainsForCurrentUser = allDomains.filter { $0.identifier.rawValue.hasSuffix("_\(user.id)") } - - let updatedDomainsForCurrentUser: [FilteredDomain] = updatedDomains.map { newDomain in - let existingDomain = existingDomainsForCurrentUser.first { $0.identifier == newDomain.identifier } - return (newDomain, existingDomain) - } - - try await updatedDomainsForCurrentUser.concurrentForEach(customConcurrency: 1) { domain in - // Simply add domain if new - let newDomain = domain.new - guard let existingDomain = domain.existing else { - try await NSFileProviderManager.add(newDomain) - return - } - - // Update existing accounts if necessary - if existingDomain.displayName != newDomain.displayName { - try await NSFileProviderManager.remove(existingDomain) - try await NSFileProviderManager.add(newDomain) - } - } - - // Remove domains no longer present for current user - let removedDomainsForCurrentUser = updatedDomains.filter { updatedDomain in - guard existingDomainsForCurrentUser.contains(where: { $0.identifier == updatedDomain.identifier }) else { - return true - } - - return false - } - - try await removedDomainsForCurrentUser.concurrentForEach(customConcurrency: 1) { oldDomain in - try await NSFileProviderManager.remove(oldDomain) - } - } catch { - DDLogError("Error while updating file provider domains: \(error)") - // TODO: add Sentry - } - - // TODO: notify for consistency - } - } - - func deleteFileProviderDomains(for userId: Int) { - NSFileProviderManager.getDomainsWithCompletionHandler { allDomains, error in - if let error { - DDLogError("Error while getting domains: \(error)") - } - - let domainsForCurrentUser = allDomains.filter { $0.identifier.rawValue.hasSuffix("_\(userId)") } - for domain in domainsForCurrentUser { - NSFileProviderManager.remove(domain) { error in - if let error { - DDLogError("Error while removing domain \(domain.displayName): \(error)") - } - } - } - } - } - - public func deleteAllFileProviderDomains() { - NSFileProviderManager.removeAllDomains { error in - if let error { - DDLogError("Error while removing domains: \(error)") - } - } - } - - func getFileProviderDomain(for driveId: String, completion: @escaping (NSFileProviderDomain?) -> Void) { - NSFileProviderManager.getDomainsWithCompletionHandler { domains, error in - if let error { - DDLogError("Error while getting domains: \(error)") - completion(nil) - } else { - completion(domains.first { $0.identifier.rawValue == driveId }) - } - } - } - - public func getFileProviderManager(for drive: Drive, completion: @escaping (NSFileProviderManager) -> Void) { - getFileProviderManager(for: drive.objectId, completion: completion) - } - - public func getFileProviderManager(driveId: Int, userId: Int, completion: @escaping (NSFileProviderManager) -> Void) { - let objectId = DriveInfosManager.getObjectId(driveId: driveId, userId: userId) - getFileProviderManager(for: objectId, completion: completion) - } - - public func getFileProviderManager(for driveId: String, completion: @escaping (NSFileProviderManager) -> Void) { - getFileProviderDomain(for: driveId) { domain in - if let domain { - completion(NSFileProviderManager(for: domain) ?? .default) - } else { - completion(.default) - } - } - } - @discardableResult func storeDriveResponse(user: InfomaniakCore.UserProfile, driveResponse: DriveResponse) -> [Drive] { var driveList = [Drive]() diff --git a/kDriveCore/Utils/FileProvider/FileProviderDomain+Identifier.swift b/kDriveCore/Utils/FileProvider/FileProviderDomain+Identifier.swift new file mode 100644 index 000000000..0c76b70f5 --- /dev/null +++ b/kDriveCore/Utils/FileProvider/FileProviderDomain+Identifier.swift @@ -0,0 +1,45 @@ +/* + Infomaniak kDrive - iOS App + Copyright (C) 2023 Infomaniak Network SA + + This program is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . + */ + +import FileProvider +import Foundation + +public extension NSFileProviderDomain { + private typealias DriveIdAndUserId = (driveId: Int, userId: Int) + + private var userAndDrive: DriveIdAndUserId? { + let identifiers = identifier.rawValue.components(separatedBy: "_") + + guard let driveIdString = identifiers[safe: 0], + let usedIdString = identifiers[safe: 1], + let driveId = Int(driveIdString), + let usedId = Int(usedIdString) else { + return nil + } + + return (driveId, usedId) + } + + var userId: Int? { + userAndDrive?.userId + } + + var driveId: Int? { + userAndDrive?.driveId + } +} From a2a04bb9aa770e1e282ff17e874017a2a1cd97cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrien=20Coye=20de=20Brune=CC=81lis?= Date: Wed, 28 Feb 2024 10:45:16 +0100 Subject: [PATCH 4/8] feat: Added an ExpiringActivity to initFileProviderDomains --- .../DriveInfosManager+FileProvider.swift | 33 ++++++++++++------- .../DriveInfosManager/DriveInfosManager.swift | 1 - 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift b/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift index 64024631d..df767c3e3 100644 --- a/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift +++ b/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift @@ -26,6 +26,9 @@ public extension DriveInfosManager { private typealias FilteredDomain = (new: NSFileProviderDomain, existing: NSFileProviderDomain?) internal func initFileProviderDomains(drives: [Drive], user: InfomaniakCore.UserProfile) { + let expiringActivity = ExpiringActivity(id: "\(#function)_\(UUID().uuidString)", delegate: nil) + expiringActivity.start() + // Clean file provider storage if needed if UserDefaults.shared.fpStorageVersion < currentFpStorageVersion { do { @@ -42,6 +45,12 @@ public extension DriveInfosManager { } } + guard !expiringActivity.shouldTerminate else { + // Sentry + expiringActivity.endAll() + return + } + // TODO: Start Activity Task { let updatedDomains = drives.map { @@ -94,7 +103,7 @@ public extension DriveInfosManager { // TODO: add Sentry } - // TODO: notify for consistency + expiringActivity.endAll() } } @@ -123,17 +132,6 @@ public extension DriveInfosManager { } } - internal func getFileProviderDomain(for driveId: String, completion: @escaping (NSFileProviderDomain?) -> Void) { - NSFileProviderManager.getDomainsWithCompletionHandler { domains, error in - if let error { - DDLogError("Error while getting domains: \(error)") - completion(nil) - } else { - completion(domains.first { $0.identifier.rawValue == driveId }) - } - } - } - func getFileProviderManager(for drive: Drive, completion: @escaping (NSFileProviderManager) -> Void) { getFileProviderManager(for: drive.objectId, completion: completion) } @@ -153,6 +151,17 @@ public extension DriveInfosManager { } } + private func getFileProviderDomain(for driveId: String, completion: @escaping (NSFileProviderDomain?) -> Void) { + NSFileProviderManager.getDomainsWithCompletionHandler { domains, error in + if let error { + DDLogError("Error while getting domains: \(error)") + completion(nil) + } else { + completion(domains.first { $0.identifier.rawValue == driveId }) + } + } + } + // MARK: Signal /// Signal changes on this Drive to the File Provider Extension diff --git a/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager.swift b/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager.swift index d60819b3f..6bbdbaf8a 100644 --- a/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager.swift +++ b/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager.swift @@ -17,7 +17,6 @@ */ import CocoaLumberjackSwift -import FileProvider import Foundation import InfomaniakCore import Realm From 96c4f7614c5d11c9ca6d6e2b280082cef468ecf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrien=20Coye=20de=20Brune=CC=81lis?= Date: Wed, 28 Feb 2024 11:20:55 +0100 Subject: [PATCH 5/8] feat: Added dedicated Log. helper for DriveInfosManager feat: Sentry logging with logger when .error level specified --- .../DriveInfosManager+FileProvider.swift | 32 +++++++++--------- .../DriveInfosManager/DriveInfosManager.swift | 12 +++---- kDriveCore/Utils/AbstractLog+Category.swift | 33 +++++++++++++++++++ kDriveCore/Utils/Sentry/SentryDebug.swift | 7 ++++ 4 files changed, 63 insertions(+), 21 deletions(-) diff --git a/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift b/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift index df767c3e3..3598c3ec8 100644 --- a/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift +++ b/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift @@ -16,7 +16,6 @@ along with this program. If not, see . */ -import CocoaLumberjackSwift import FileProvider import Foundation import InfomaniakConcurrency @@ -41,17 +40,16 @@ public extension DriveInfosManager { } UserDefaults.shared.fpStorageVersion = currentFpStorageVersion } catch { - // TODO: Sentry + Log.driveInfosManager("FileManager issue :\(error)", level: .error) } } guard !expiringActivity.shouldTerminate else { - // Sentry + Log.driveInfosManager("Expiring activity before signalling FileProvider", level: .error) expiringActivity.endAll() return } - // TODO: Start Activity Task { let updatedDomains = drives.map { NSFileProviderDomain( @@ -99,8 +97,7 @@ public extension DriveInfosManager { try await NSFileProviderManager.remove(oldDomain) } } catch { - DDLogError("Error while updating file provider domains: \(error)") - // TODO: add Sentry + Log.driveInfosManager("Error while updating file provider domains: \(error)", level: .error) } expiringActivity.endAll() @@ -110,15 +107,16 @@ public extension DriveInfosManager { internal func deleteFileProviderDomains(for userId: Int) { NSFileProviderManager.getDomainsWithCompletionHandler { allDomains, error in if let error { - DDLogError("Error while getting domains: \(error)") + Log.driveInfosManager("Error while getting domains: \(error)", level: .error) } let domainsForCurrentUser = allDomains.filter { $0.identifier.rawValue.hasSuffix("_\(userId)") } for domain in domainsForCurrentUser { NSFileProviderManager.remove(domain) { error in - if let error { - DDLogError("Error while removing domain \(domain.displayName): \(error)") + guard let error else { + return } + Log.driveInfosManager("Error while removing domain \(domain.displayName): \(error)", level: .error) } } } @@ -126,9 +124,10 @@ public extension DriveInfosManager { func deleteAllFileProviderDomains() { NSFileProviderManager.removeAllDomains { error in - if let error { - DDLogError("Error while removing domains: \(error)") + guard let error else { + return } + Log.driveInfosManager("Error while removing domains: \(error)", level: .error) } } @@ -154,7 +153,7 @@ public extension DriveInfosManager { private func getFileProviderDomain(for driveId: String, completion: @escaping (NSFileProviderDomain?) -> Void) { NSFileProviderManager.getDomainsWithCompletionHandler { domains, error in if let error { - DDLogError("Error while getting domains: \(error)") + Log.driveInfosManager("Error while getting domains: \(error)", level: .error) completion(nil) } else { completion(domains.first { $0.identifier.rawValue == driveId }) @@ -167,16 +166,19 @@ public extension DriveInfosManager { /// Signal changes on this Drive to the File Provider Extension private func signalChanges(for domain: NSFileProviderDomain) { guard let driveId = domain.driveId, let userId = domain.userId else { - // Sentry + Log.driveInfosManager( + "Unable to read: driveId:\(String(describing: domain.driveId)) userId:\(String(describing: domain.userId))", + level: .error + ) return } DriveInfosManager.instance.getFileProviderManager(driveId: driveId, userId: userId) { manager in manager.signalEnumerator(for: .workingSet) { _ in - // META: keep SonarCloud happy + Log.driveInfosManager("did signal .workingSet") } manager.signalEnumerator(for: .rootContainer) { _ in - // META: keep SonarCloud happy + Log.driveInfosManager("did signal .rootContainer") } } } diff --git a/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager.swift b/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager.swift index 6bbdbaf8a..60984ed0d 100644 --- a/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager.swift +++ b/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager.swift @@ -16,7 +16,6 @@ along with this program. If not, see . */ -import CocoaLumberjackSwift import Foundation import InfomaniakCore import Realm @@ -24,15 +23,16 @@ import RealmSwift import Sentry public final class DriveInfosManager { - // TODO: use DI - public static let instance = DriveInfosManager() + private static let dbName = "DrivesInfos.realm" + private static let currentDbVersion: UInt64 = 9 let currentFpStorageVersion = 1 public let realmConfiguration: Realm.Configuration - private let dbName = "DrivesInfos.realm" - private var fileProviderManagers: [String: NSFileProviderManager] = [:] + + // TODO: use DI + public static let instance = DriveInfosManager() private class func removeDanglingObjects(ofType type: RLMObjectBase.Type, migration: Migration, ids: Set) { migration.enumerateObjects(ofType: type.className()) { oldObject, newObject in @@ -45,7 +45,7 @@ public final class DriveInfosManager { private init() { realmConfiguration = Realm.Configuration( - fileURL: DriveFileManager.constants.rootDocumentsURL.appendingPathComponent(dbName), + fileURL: DriveFileManager.constants.rootDocumentsURL.appendingPathComponent(Self.dbName), schemaVersion: DriveInfosManager.currentDbVersion, migrationBlock: { migration, oldSchemaVersion in if oldSchemaVersion < DriveInfosManager.currentDbVersion { diff --git a/kDriveCore/Utils/AbstractLog+Category.swift b/kDriveCore/Utils/AbstractLog+Category.swift index 7cafd1bed..8b1b4323b 100644 --- a/kDriveCore/Utils/AbstractLog+Category.swift +++ b/kDriveCore/Utils/AbstractLog+Category.swift @@ -175,6 +175,39 @@ public enum Log { tag: tag) } + /// Shorthand for ABLog, with "DriveInfosManager" category. + /// + /// Sentry tracking enabled when level == .error + public static func driveInfosManager(_ message: @autoclosure () -> Any, + level: AbstractLogLevel = .debug, + context: Int = 0, + file: StaticString = #file, + function: StaticString = #function, + line: UInt = #line, + tag: Any? = nil) { + let category = "DriveInfosManager" + let messageAny = message() + guard let messageString = messageAny as? String else { + assertionFailure("This should always cast to a String") + return + } + + // All errors are tracked on Sentry + if level == .error { + SentryDebug.addBreadcrumb(message: messageString, category: .DriveInfosManager, level: .error) + SentryDebug.capture(message: messageString, level: .error, extras: ["function": "\(function)", "line": "\(line)"]) + } + + ABLog(messageAny, + category: category, + level: level, + context: context, + file: file, + function: function, + line: line, + tag: tag) + } + private static func defaultLogHandler(_ message: @autoclosure () -> Any, category: String, level: AbstractLogLevel, diff --git a/kDriveCore/Utils/Sentry/SentryDebug.swift b/kDriveCore/Utils/Sentry/SentryDebug.swift index 20b4bc3e4..2d1decb03 100644 --- a/kDriveCore/Utils/Sentry/SentryDebug.swift +++ b/kDriveCore/Utils/Sentry/SentryDebug.swift @@ -38,6 +38,8 @@ public enum SentryDebug { case realmMigration = "RealmMigration" /// Photo library assets case PHAsset + /// DriveInfosManager, and communication with FileProvider APIs + case DriveInfosManager } public enum EventNames { @@ -136,6 +138,7 @@ public enum SentryDebug { message: String, context: [String: Any]? = nil, contextKey: String? = nil, + level: SentryLevel? = nil, extras: [String: Any]? = nil ) { Task { @@ -144,6 +147,10 @@ public enum SentryDebug { scope.setContext(value: context, key: contextKey) } + if let level { + scope.setLevel(level) + } + if let extras { scope.setExtras(extras) } From 38a9ca4451a8b9eaa6dee15a7943af94e725ee53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrien=20Coye=20de=20Brune=CC=81lis?= Date: Wed, 28 Feb 2024 11:30:30 +0100 Subject: [PATCH 6/8] chore: Sonar Cloud feedback --- .../DriveInfosManager+FileProvider.swift | 116 ++++++++++-------- 1 file changed, 63 insertions(+), 53 deletions(-) diff --git a/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift b/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift index 3598c3ec8..670d96045 100644 --- a/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift +++ b/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift @@ -50,58 +50,7 @@ public extension DriveInfosManager { return } - Task { - let updatedDomains = drives.map { - NSFileProviderDomain( - identifier: NSFileProviderDomainIdentifier($0.objectId), - displayName: "\($0.name) (\(user.email))", - pathRelativeToDocumentStorage: "\($0.objectId)" - ) - } - - do { - let allDomains = try await NSFileProviderManager.domains() - let existingDomainsForCurrentUser = allDomains.filter { $0.identifier.rawValue.hasSuffix("_\(user.id)") } - - let updatedDomainsForCurrentUser: [FilteredDomain] = updatedDomains.map { newDomain in - let existingDomain = existingDomainsForCurrentUser.first { $0.identifier == newDomain.identifier } - return (newDomain, existingDomain) - } - - try await updatedDomainsForCurrentUser.concurrentForEach(customConcurrency: 1) { domain in - // Simply add domain if new - let newDomain = domain.new - guard let existingDomain = domain.existing else { - try await NSFileProviderManager.add(newDomain) - return - } - - // Update existing accounts if necessary - if existingDomain.displayName != newDomain.displayName { - try await NSFileProviderManager.remove(existingDomain) - try await NSFileProviderManager.add(newDomain) - self.signalChanges(for: newDomain) - } - } - - // Remove domains no longer present for current user - let removedDomainsForCurrentUser = updatedDomains.filter { updatedDomain in - guard existingDomainsForCurrentUser.contains(where: { $0.identifier == updatedDomain.identifier }) else { - return true - } - - return false - } - - try await removedDomainsForCurrentUser.concurrentForEach(customConcurrency: 1) { oldDomain in - try await NSFileProviderManager.remove(oldDomain) - } - } catch { - Log.driveInfosManager("Error while updating file provider domains: \(error)", level: .error) - } - - expiringActivity.endAll() - } + updateFileManagerDomains(drives: drives, user: user, expiringActivity: expiringActivity) } internal func deleteFileProviderDomains(for userId: Int) { @@ -161,7 +110,68 @@ public extension DriveInfosManager { } } - // MARK: Signal + // MARK: Update FileManager + + /// Diffing __NSFileProviderDomain__ for drives of a specified user, and propagate changes to the __NSFileProviderManager__ + private func updateFileManagerDomains(drives: [Drive], user: InfomaniakCore.UserProfile, expiringActivity: ExpiringActivity) { + Task { + let updatedDomains = drives.map { + NSFileProviderDomain( + identifier: NSFileProviderDomainIdentifier($0.objectId), + displayName: "\($0.name) (\(user.email))", + pathRelativeToDocumentStorage: "\($0.objectId)" + ) + } + + do { + let allDomains = try await NSFileProviderManager.domains() + let existingDomainsForCurrentUser = allDomains.filter { $0.identifier.rawValue.hasSuffix("_\(user.id)") } + + let updatedDomainsForCurrentUser: [FilteredDomain] = updatedDomains.map { newDomain in + let existingDomain = existingDomainsForCurrentUser.first { $0.identifier == newDomain.identifier } + return (newDomain, existingDomain) + } + + try await updatedDomainsForCurrentUser.concurrentForEach(customConcurrency: 1) { domain in + // Simply add domain if new + let newDomain = domain.new + guard let existingDomain = domain.existing else { + Log.driveInfosManager("Inserting new domain:\(newDomain.identifier)") + try await NSFileProviderManager.add(newDomain) + return + } + + // Update existing accounts if necessary + if existingDomain.displayName != newDomain.displayName { + Log.driveInfosManager("Updating domain:\(newDomain.identifier)") + try await NSFileProviderManager.remove(existingDomain) + try await NSFileProviderManager.add(newDomain) + self.signalChanges(for: newDomain) + } + } + + // Remove domains no longer present for current user + let removedDomainsForCurrentUser = updatedDomains.filter { updatedDomain in + guard existingDomainsForCurrentUser.contains(where: { $0.identifier == updatedDomain.identifier }) else { + return true + } + + return false + } + + try await removedDomainsForCurrentUser.concurrentForEach(customConcurrency: 1) { oldDomain in + Log.driveInfosManager("Removing domain:\(oldDomain.identifier)") + try await NSFileProviderManager.remove(oldDomain) + } + } catch { + Log.driveInfosManager("Error while updating file provider domains: \(error)", level: .error) + } + + expiringActivity.endAll() + } + } + + // MARK: Signal FileManager /// Signal changes on this Drive to the File Provider Extension private func signalChanges(for domain: NSFileProviderDomain) { From de6e61506bd8fc6066fdeb127af17b1cb466c092 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrien=20Coye=20de=20Brune=CC=81lis?= Date: Wed, 28 Feb 2024 13:22:57 +0100 Subject: [PATCH 7/8] fix: Reimplementation of the diffing algorithm works refactor: Split some more DriveInfosManager+FileProvider --- kDriveCore/DI/FactoryService.swift | 3 +- .../DriveInfosManager+FileProvider.swift | 125 +++++++++++------- 2 files changed, 76 insertions(+), 52 deletions(-) diff --git a/kDriveCore/DI/FactoryService.swift b/kDriveCore/DI/FactoryService.swift index cf10e9340..6663b920d 100644 --- a/kDriveCore/DI/FactoryService.swift +++ b/kDriveCore/DI/FactoryService.swift @@ -158,7 +158,8 @@ public enum FactoryService { (loggerFactory, "BGTaskScheduling"), (loggerFactory, "PhotoLibraryUploader"), (loggerFactory, "AppDelegate"), - (loggerFactory, "FileProvider") + (loggerFactory, "FileProvider"), + (loggerFactory, "DriveInfosManager") ] return services } else { diff --git a/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift b/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift index 670d96045..a97febd5d 100644 --- a/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift +++ b/kDriveCore/Data/Cache/DriveInfosManager/DriveInfosManager+FileProvider.swift @@ -25,9 +25,6 @@ public extension DriveInfosManager { private typealias FilteredDomain = (new: NSFileProviderDomain, existing: NSFileProviderDomain?) internal func initFileProviderDomains(drives: [Drive], user: InfomaniakCore.UserProfile) { - let expiringActivity = ExpiringActivity(id: "\(#function)_\(UUID().uuidString)", delegate: nil) - expiringActivity.start() - // Clean file provider storage if needed if UserDefaults.shared.fpStorageVersion < currentFpStorageVersion { do { @@ -44,13 +41,7 @@ public extension DriveInfosManager { } } - guard !expiringActivity.shouldTerminate else { - Log.driveInfosManager("Expiring activity before signalling FileProvider", level: .error) - expiringActivity.endAll() - return - } - - updateFileManagerDomains(drives: drives, user: user, expiringActivity: expiringActivity) + updateFileManagerDomains(drives: drives, user: user) } internal func deleteFileProviderDomains(for userId: Int) { @@ -74,6 +65,7 @@ public extension DriveInfosManager { func deleteAllFileProviderDomains() { NSFileProviderManager.removeAllDomains { error in guard let error else { + Log.driveInfosManager("Did remove all domains") return } Log.driveInfosManager("Error while removing domains: \(error)", level: .error) @@ -113,7 +105,9 @@ public extension DriveInfosManager { // MARK: Update FileManager /// Diffing __NSFileProviderDomain__ for drives of a specified user, and propagate changes to the __NSFileProviderManager__ - private func updateFileManagerDomains(drives: [Drive], user: InfomaniakCore.UserProfile, expiringActivity: ExpiringActivity) { + private func updateFileManagerDomains(drives: [Drive], user: InfomaniakCore.UserProfile) { + let expiringActivity = ExpiringActivity(id: "\(#function)_\(UUID().uuidString)", delegate: nil) + expiringActivity.start() Task { let updatedDomains = drives.map { NSFileProviderDomain( @@ -123,54 +117,74 @@ public extension DriveInfosManager { ) } + Log.driveInfosManager("Updated domains \(updatedDomains.count) for user :\(user.displayName) \(user.id)") do { - let allDomains = try await NSFileProviderManager.domains() - let existingDomainsForCurrentUser = allDomains.filter { $0.identifier.rawValue.hasSuffix("_\(user.id)") } + try await updateDomainsIfNecessary(updatedDomains: updatedDomains, userId: user.id) + try await deleteDomainsIfNecessary(updatedDomains: updatedDomains, userId: user.id) + } catch { + Log.driveInfosManager("Error while updating file provider domains: \(error)", level: .error) + } - let updatedDomainsForCurrentUser: [FilteredDomain] = updatedDomains.map { newDomain in - let existingDomain = existingDomainsForCurrentUser.first { $0.identifier == newDomain.identifier } - return (newDomain, existingDomain) - } + expiringActivity.endAll() + } + } - try await updatedDomainsForCurrentUser.concurrentForEach(customConcurrency: 1) { domain in - // Simply add domain if new - let newDomain = domain.new - guard let existingDomain = domain.existing else { - Log.driveInfosManager("Inserting new domain:\(newDomain.identifier)") - try await NSFileProviderManager.add(newDomain) - return - } + /// Insert or update Domains if necessary + private func updateDomainsIfNecessary(updatedDomains: [NSFileProviderDomain], userId: Int) async throws { + let existingDomainsForCurrentUser = try await existingDomains(for: userId) - // Update existing accounts if necessary - if existingDomain.displayName != newDomain.displayName { - Log.driveInfosManager("Updating domain:\(newDomain.identifier)") - try await NSFileProviderManager.remove(existingDomain) - try await NSFileProviderManager.add(newDomain) - self.signalChanges(for: newDomain) - } - } + let updatedDomainsForCurrentUser: [FilteredDomain] = updatedDomains.map { newDomain in + let existingDomain = existingDomainsForCurrentUser.first { $0.identifier == newDomain.identifier } + return (newDomain, existingDomain) + } - // Remove domains no longer present for current user - let removedDomainsForCurrentUser = updatedDomains.filter { updatedDomain in - guard existingDomainsForCurrentUser.contains(where: { $0.identifier == updatedDomain.identifier }) else { - return true - } + try await updatedDomainsForCurrentUser.concurrentForEach(customConcurrency: 1) { domain in + // Simply add domain if new + let newDomain = domain.new + guard let existingDomain = domain.existing else { + Log.driveInfosManager("Inserting new domain:\(newDomain.identifier)") + try await NSFileProviderManager.add(newDomain) + self.signalChanges(for: newDomain) + return + } - return false - } + // Update existing accounts if necessary + if existingDomain.displayName != newDomain.displayName { + Log.driveInfosManager("Updating domain:\(newDomain.identifier)") + try await NSFileProviderManager.remove(existingDomain) + try await NSFileProviderManager.add(newDomain) + self.signalChanges(for: newDomain) + } + } + } - try await removedDomainsForCurrentUser.concurrentForEach(customConcurrency: 1) { oldDomain in - Log.driveInfosManager("Removing domain:\(oldDomain.identifier)") - try await NSFileProviderManager.remove(oldDomain) - } - } catch { - Log.driveInfosManager("Error while updating file provider domains: \(error)", level: .error) + /// Delete Domains if necessary + private func deleteDomainsIfNecessary(updatedDomains: [NSFileProviderDomain], userId: Int) async throws { + // We need to fetch a fresh copy of the domains after the update + let existingDomainsForCurrentUser = try await existingDomains(for: userId) + + // Remove domains no longer present for current user + let removedDomainsForCurrentUser = updatedDomains.filter { updatedDomain in + guard existingDomainsForCurrentUser.contains(where: { $0.identifier == updatedDomain.identifier }) else { + return true } - expiringActivity.endAll() + return false + } + + try await removedDomainsForCurrentUser.concurrentForEach(customConcurrency: 1) { oldDomain in + Log.driveInfosManager("Removing domain:\(oldDomain.identifier)") + try await NSFileProviderManager.remove(oldDomain) } } + /// Fetch a fresh list of registered domains for a specified user + private func existingDomains(for userId: Int) async throws -> [NSFileProviderDomain] { + let allDomains = try await NSFileProviderManager.domains() + let existingDomains = allDomains.filter { $0.identifier.rawValue.hasSuffix("_\(userId)") } + return existingDomains + } + // MARK: Signal FileManager /// Signal changes on this Drive to the File Provider Extension @@ -184,11 +198,20 @@ public extension DriveInfosManager { } DriveInfosManager.instance.getFileProviderManager(driveId: driveId, userId: userId) { manager in - manager.signalEnumerator(for: .workingSet) { _ in - Log.driveInfosManager("did signal .workingSet") + manager.signalEnumerator(for: .workingSet) { error in + guard let error else { + Log.driveInfosManager("did signal .workingSet") + return + } + + Log.driveInfosManager("failed to signal .workingSet \(error)", level: .error) } - manager.signalEnumerator(for: .rootContainer) { _ in - Log.driveInfosManager("did signal .rootContainer") + manager.signalEnumerator(for: .rootContainer) { error in + guard let error else { + Log.driveInfosManager("did signal .rootContainer") + return + } + Log.driveInfosManager("failed to signal .rootContainer \(error)", level: .error) } } } From ce4148bb6671001c5233df6433edca2694a37530 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrien=20Coye=20de=20Brune=CC=81lis?= Date: Wed, 28 Feb 2024 14:02:03 +0100 Subject: [PATCH 8/8] fix: Unit test DI --- kDriveTests/kDrive/Launch/ITAppLaunchTest.swift | 4 ++++ kDriveTests/kDrive/Launch/UTRootViewControllerState.swift | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/kDriveTests/kDrive/Launch/ITAppLaunchTest.swift b/kDriveTests/kDrive/Launch/ITAppLaunchTest.swift index f9cbab131..460c8bd21 100644 --- a/kDriveTests/kDrive/Launch/ITAppLaunchTest.swift +++ b/kDriveTests/kDrive/Launch/ITAppLaunchTest.swift @@ -43,6 +43,10 @@ final class ITAppLaunchTest: XCTestCase { SimpleResolver.register(FactoryService.debugServices) let services = [ + Factory(type: AppContextServiceable.self) { _, _ in + // We fake the main app context + return AppContextService(context: .app) + }, Factory(type: InfomaniakNetworkLogin.self) { _, _ in return InfomaniakNetworkLogin(config: self.loginConfig) }, diff --git a/kDriveTests/kDrive/Launch/UTRootViewControllerState.swift b/kDriveTests/kDrive/Launch/UTRootViewControllerState.swift index e915f0080..f66a4f11c 100644 --- a/kDriveTests/kDrive/Launch/UTRootViewControllerState.swift +++ b/kDriveTests/kDrive/Launch/UTRootViewControllerState.swift @@ -42,6 +42,10 @@ final class UTRootViewControllerState: XCTestCase { SimpleResolver.sharedResolver.removeAll() let services = [ + Factory(type: AppContextServiceable.self) { _, _ in + // We fake the main app context + return AppContextService(context: .app) + }, Factory(type: InfomaniakNetworkLogin.self) { _, _ in InfomaniakNetworkLogin(config: self.loginConfig) },