diff --git a/DuckDuckGo.xcodeproj/project.pbxproj b/DuckDuckGo.xcodeproj/project.pbxproj index dd2b20e00e..6b85344b23 100644 --- a/DuckDuckGo.xcodeproj/project.pbxproj +++ b/DuckDuckGo.xcodeproj/project.pbxproj @@ -1432,6 +1432,8 @@ 9833913327AAAEEE00DAF119 /* EmbeddedTrackerDataTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9833913227AAAEEE00DAF119 /* EmbeddedTrackerDataTests.swift */; }; 983DFB2528B67036006B7E34 /* UserContentUpdating.swift in Sources */ = {isa = PBXBuildFile; fileRef = 983DFB2428B67036006B7E34 /* UserContentUpdating.swift */; }; 984FD3BF299ACF35007334DD /* Bookmarks in Frameworks */ = {isa = PBXBuildFile; productRef = 984FD3BE299ACF35007334DD /* Bookmarks */; }; + 986189E62A7CFB3E001B4519 /* LocalBookmarkStoreSavingTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 986189E52A7CFB3E001B4519 /* LocalBookmarkStoreSavingTests.swift */; }; + 986189E72A7CFB3E001B4519 /* LocalBookmarkStoreSavingTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 986189E52A7CFB3E001B4519 /* LocalBookmarkStoreSavingTests.swift */; }; 987799ED299998B1005D8EB6 /* Bookmarks in Frameworks */ = {isa = PBXBuildFile; productRef = 987799EC299998B1005D8EB6 /* Bookmarks */; }; 987799F12999993C005D8EB6 /* LegacyBookmarkStore.swift in Sources */ = {isa = PBXBuildFile; fileRef = 987799EF2999993C005D8EB6 /* LegacyBookmarkStore.swift */; }; 987799F22999993C005D8EB6 /* LegacyBookmarkStore.swift in Sources */ = {isa = PBXBuildFile; fileRef = 987799EF2999993C005D8EB6 /* LegacyBookmarkStore.swift */; }; @@ -2661,6 +2663,7 @@ 9833913027AAA4B500DAF119 /* trackerData.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = trackerData.json; sourceTree = ""; }; 9833913227AAAEEE00DAF119 /* EmbeddedTrackerDataTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EmbeddedTrackerDataTests.swift; sourceTree = ""; }; 983DFB2428B67036006B7E34 /* UserContentUpdating.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = UserContentUpdating.swift; sourceTree = ""; }; + 986189E52A7CFB3E001B4519 /* LocalBookmarkStoreSavingTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LocalBookmarkStoreSavingTests.swift; sourceTree = ""; }; 987799EF2999993C005D8EB6 /* LegacyBookmarkStore.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = LegacyBookmarkStore.swift; sourceTree = ""; }; 987799F02999993C005D8EB6 /* LegacyBookmarksStoreMigration.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = LegacyBookmarksStoreMigration.swift; sourceTree = ""; }; 987799F52999996B005D8EB6 /* BookmarkDatabase.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = BookmarkDatabase.swift; sourceTree = ""; }; @@ -5197,6 +5200,7 @@ isa = PBXGroup; children = ( AA652CB025DD825B009059CC /* LocalBookmarkStoreTests.swift */, + 986189E52A7CFB3E001B4519 /* LocalBookmarkStoreSavingTests.swift */, AA652CDA25DDAB32009059CC /* BookmarkStoreMock.swift */, ); path = Services; @@ -8152,6 +8156,7 @@ 3706FE3F293F661700E42796 /* FileStoreMock.swift in Sources */, 3706FE40293F661700E42796 /* BWResponseTests.swift in Sources */, 3706FE41293F661700E42796 /* DownloadListCoordinatorTests.swift in Sources */, + 986189E72A7CFB3E001B4519 /* LocalBookmarkStoreSavingTests.swift in Sources */, 3706FE42293F661700E42796 /* BWMessageIdGeneratorTests.swift in Sources */, 3706FE43293F661700E42796 /* TestDataModel.xcdatamodeld in Sources */, 3706FE44293F661700E42796 /* GeolocationServiceTests.swift in Sources */, @@ -9171,6 +9176,7 @@ B69B50452726C5C200758A2B /* AtbParserTests.swift in Sources */, B6106BAF26A7C6180013B453 /* PermissionStoreMock.swift in Sources */, 4B98D27A28D95F1A003C2B6F /* ChromiumFaviconsReaderTests.swift in Sources */, + 986189E62A7CFB3E001B4519 /* LocalBookmarkStoreSavingTests.swift in Sources */, AA652CD325DDA6E9009059CC /* LocalBookmarkManagerTests.swift in Sources */, CBDD5DE329A67F2700832877 /* MockConfigurationStore.swift in Sources */, B63ED0DC26AE7B1E00A9DAD1 /* WebViewMock.swift in Sources */, diff --git a/DuckDuckGo/Bookmarks/Services/LocalBookmarkStore.swift b/DuckDuckGo/Bookmarks/Services/LocalBookmarkStore.swift index 7899280329..6940ac217f 100644 --- a/DuckDuckGo/Bookmarks/Services/LocalBookmarkStore.swift +++ b/DuckDuckGo/Bookmarks/Services/LocalBookmarkStore.swift @@ -22,21 +22,23 @@ import CoreData import DDGSync import Bookmarks import Cocoa +import Persistence // swiftlint:disable:next type_body_length final class LocalBookmarkStore: BookmarkStore { - init(bookmarkDatabase: BookmarkDatabase) { - self.context = bookmarkDatabase.db.makeContext(concurrencyType: .privateQueueConcurrencyType) - sharedInitialization() + convenience init(bookmarkDatabase: BookmarkDatabase) { + self.init(contextProvider: { + let context = bookmarkDatabase.db.makeContext(concurrencyType: .privateQueueConcurrencyType) + context.stalenessInterval = 0 + return context + }) } - init(context: NSManagedObjectContext) { - self.context = context - sharedInitialization() - } + // Directly used in tests + init(contextProvider: @escaping () -> NSManagedObjectContext) { + self.contextProvider = contextProvider - private func sharedInitialization() { removeInvalidBookmarkEntities() cacheReadOnlyTopLevelBookmarksFolders() } @@ -47,24 +49,161 @@ final class LocalBookmarkStore: BookmarkStore { case badObjectId case asyncFetchFailed case missingParent + case missingEntity + case missingRoot + case missingFavoritesRoot + case saveLoopError(Error?) } - private let context: NSManagedObjectContext + private let contextProvider: () -> NSManagedObjectContext /// All entities within the bookmarks store must exist under this root level folder. Because this value is used so frequently, it is cached here. - private var rootLevelFolder: BookmarkEntity? + private var rootLevelFolderObjectID: NSManagedObjectID? /// All favorites must additionally be children of this special folder. Because this value is used so frequently, it is cached here. - private var favoritesFolder: BookmarkEntity? + private var favoritesFolderObjectID: NSManagedObjectID? + + private func makeContext() -> NSManagedObjectContext { + return contextProvider() + } + + private func bookmarksRoot(in context: NSManagedObjectContext) -> BookmarkEntity? { + guard let objectID = self.rootLevelFolderObjectID else { + return nil + } + return try? (context.existingObject(with: objectID) as? BookmarkEntity) + } + + private func favoritesRoot(in context: NSManagedObjectContext) -> BookmarkEntity? { + guard let objectID = self.favoritesFolderObjectID else { + return nil + } + return try? (context.existingObject(with: objectID) as? BookmarkEntity) + } + + /* + Utility function to help with saving changes to the database. + + If there is a timing issue (e.g. another context making changes), we may encounter merge error on save, in such case: + - reset context + - reapply changes + - retry save + + You can expect either `onError` or `onDidSave` to be called once. + Error thrown from within `changes` block triggers `onError` call and prevents saving. + */ + func applyChangesAndSave(changes: @escaping (NSManagedObjectContext) throws -> Void, + onError: @escaping (Error) -> Void, + onDidSave: @escaping () -> Void) { + let context = makeContext() + + let maxRetries = 4 + var iteration = 0 + + context.perform { + var lastError: Error? + while iteration < maxRetries { + do { + try changes(context) + + try context.save() + onDidSave() + return + } catch { + let nsError = error as NSError + if nsError.code == NSManagedObjectMergeError || nsError.code == NSManagedObjectConstraintMergeError { + iteration += 1 + lastError = error + context.reset() + } else { + onError(error) + return + } + } + } + + onError(BookmarkStoreError.saveLoopError(lastError)) + } + } + + /* + Utility function to help with saving changes to the database. + + If there is a timing issue (e.g. another context making changes), we may encounter merge error on save, in such case: + - reset context + - reapply changes + - retry save + + Error thrown from within `changes` block prevent saving and is rethrown. + */ + func applyChangesAndSave(changes: (NSManagedObjectContext) throws -> Void) throws { + let context = makeContext() + + let maxRetries = 4 + var iteration = 0 + + var errorToThrow: Error? + context.performAndWait { + var lastMergeError: NSError? + while iteration < maxRetries { + do { + try changes(context) + try context.save() + return + } catch { + + let nsError = error as NSError + if nsError.code == NSManagedObjectMergeError || nsError.code == NSManagedObjectConstraintMergeError { + lastMergeError = nsError + iteration += 1 + context.reset() + } else { + errorToThrow = error + return + } + } + } + + errorToThrow = BookmarkStoreError.saveLoopError(lastMergeError) + } + + if let errorToThrow { + throw errorToThrow + } + } + + private func commonOnSaveErrorHandler(_ error: Error) { + guard !NSApp.isRunningUnitTests else { return } + + assertionFailure("LocalBookmarkStore: Saving of context failed") + + if let localError = error as? BookmarkStoreError { + if case BookmarkStoreError.saveLoopError(let innerError) = localError, let innerError { + let processedErrors = CoreDataErrorsParser.parse(error: innerError as NSError) + + Pixel.fire(.debug(event: .bookmarksSaveFailed, error: error), + withAdditionalParameters: processedErrors.errorPixelParameters) + } else { + Pixel.fire(.debug(event: .bookmarksSaveFailed, error: localError)) + } + } else { + let error = error as NSError + let processedErrors = CoreDataErrorsParser.parse(error: error) + + Pixel.fire(.debug(event: .bookmarksSaveFailed, error: error), + withAdditionalParameters: processedErrors.errorPixelParameters) + } + } private func cacheReadOnlyTopLevelBookmarksFolders() { + let context = makeContext() context.performAndWait { guard let folder = BookmarkUtils.fetchRootFolder(context) else { fatalError("Top level folder missing") } - self.rootLevelFolder = folder - self.favoritesFolder = BookmarkUtils.fetchFavoritesFolder(context) + self.rootLevelFolderObjectID = folder.objectID + self.favoritesFolderObjectID = BookmarkUtils.fetchFavoritesFolder(context)?.objectID } } @@ -77,27 +216,27 @@ final class LocalBookmarkStore: BookmarkStore { } } + let context = makeContext() context.perform { do { - self.context.refreshAllObjects() let results: [BookmarkEntity] switch type { case .bookmarks: let fetchRequest = Bookmark.bookmarksFetchRequest() fetchRequest.returnsObjectsAsFaults = false - results = try self.context.fetch(fetchRequest) + results = try context.fetch(fetchRequest) case .topLevelEntities: // When fetching the top level entities, the root folder will be returned. To make things simpler for the caller, this function // will return the children of the root folder, as the root folder is an implementation detail of the bookmarks store. - let rootFolder = BookmarkUtils.fetchRootFolder(self.context) - let orphanedEntities = BookmarkUtils.fetchOrphanedEntities(self.context) + let rootFolder = self.bookmarksRoot(in: context) + let orphanedEntities = BookmarkUtils.fetchOrphanedEntities(context) if !orphanedEntities.isEmpty { self.reportOrphanedBookmarksIfNeeded() } results = (rootFolder?.childrenArray ?? []) + orphanedEntities case .favorites: - results = self.favoritesFolder?.favoritesArray ?? [] + results = self.favoritesRoot(in: context)?.favoritesArray ?? [] } let entities: [BaseBookmarkEntity] = results.compactMap { entity in @@ -122,65 +261,57 @@ final class LocalBookmarkStore: BookmarkStore { } func save(bookmark: Bookmark, parent: BookmarkFolder?, index: Int?, completion: @escaping (Bool, Error?) -> Void) { - context.perform { [weak self] in + applyChangesAndSave(changes: { [weak self] context in guard let self = self else { - DispatchQueue.main.async { completion(false, BookmarkStoreError.storeDeallocated) } - return + throw BookmarkStoreError.storeDeallocated } let parentEntity: BookmarkEntity if let parent = parent, - let parentFetchRequestResult = try? self.context.fetch(BaseBookmarkEntity.singleEntity(with: parent.id)).first { + let parentFetchRequestResult = try? context.fetch(BaseBookmarkEntity.singleEntity(with: parent.id)).first { parentEntity = parentFetchRequestResult - } else if let root = BookmarkUtils.fetchRootFolder(self.context) { + } else if let root = bookmarksRoot(in: context) { parentEntity = root } else { Pixel.fire(.debug(event: .missingParent)) - DispatchQueue.main.async { completion(false, BookmarkStoreError.missingParent) } - return + throw BookmarkStoreError.missingParent } let bookmarkMO: BookmarkEntity if bookmark.isFolder { bookmarkMO = BookmarkEntity.makeFolder(title: bookmark.title, parent: parentEntity, - context: self.context) + context: context) } else { bookmarkMO = BookmarkEntity.makeBookmark(title: bookmark.title, url: bookmark.url, parent: parentEntity, - context: self.context) + context: context) if bookmark.isFavorite, - let favoritesFolder = self.favoritesFolder { + let favoritesFolder = self.favoritesRoot(in: context) { bookmarkMO.addToFavorites(favoritesRoot: favoritesFolder) } } bookmarkMO.uuid = bookmark.id - - do { - try self.context.save(onErrorFire: .bookmarksSaveFailed) - } catch { - self.context.rollback() - assertionFailure("LocalBookmarkStore: Saving of context failed") - DispatchQueue.main.async { completion(false, error) } - return - } - + }, onError: { [weak self] error in + self?.commonOnSaveErrorHandler(error) + DispatchQueue.main.async { completion(false, error) } + }, onDidSave: { DispatchQueue.main.async { completion(true, nil) } - } + }) } func remove(objectsWithUUIDs identifiers: [String], completion: @escaping (Bool, Error?) -> Void) { - context.perform { [weak self] in + + applyChangesAndSave(changes: { [weak self] context in guard let self = self else { - DispatchQueue.main.async { completion(false, BookmarkStoreError.storeDeallocated) } - return + throw BookmarkStoreError.storeDeallocated } let fetchRequest = BaseBookmarkEntity.entities(with: identifiers) - let fetchResults = (try? self.context.fetch(fetchRequest)) ?? [] + let fetchResults = (try? context.fetch(fetchRequest)) ?? [] if fetchResults.count != identifiers.count { assertionFailure("\(#file): Fetched bookmark entities didn't match the number of provided UUIDs") @@ -189,195 +320,161 @@ final class LocalBookmarkStore: BookmarkStore { for object in fetchResults { object.markPendingDeletion() } - - do { - try self.context.save(onErrorFire: .bookmarksSaveFailed) - } catch { - self.context.rollback() - assertionFailure("LocalBookmarkStore: Saving of context failed") - DispatchQueue.main.async { completion(false, error) } - } - + }, onError: { [weak self] error in + self?.commonOnSaveErrorHandler(error) + DispatchQueue.main.async { completion(false, error) } + }, onDidSave: { DispatchQueue.main.async { completion(true, nil) } - } + }) } func update(bookmark: Bookmark) { - context.performAndWait { [weak self] in - guard let self = self else { - return - } - let bookmarkFetchRequest = BaseBookmarkEntity.singleEntity(with: bookmark.id) - let bookmarkFetchRequestResults = try? self.context.fetch(bookmarkFetchRequest) + do { + try applyChangesAndSave(changes: { [weak self] context in + guard let self = self else { + throw BookmarkStoreError.storeDeallocated + } - guard let bookmarkMO = bookmarkFetchRequestResults?.first else { - assertionFailure("LocalBookmarkStore: Failed to get BookmarkEntity from the context") - return - } + let bookmarkFetchRequest = BaseBookmarkEntity.singleEntity(with: bookmark.id) + let bookmarkFetchRequestResults = try? context.fetch(bookmarkFetchRequest) - bookmarkMO.update(with: bookmark, favoritesFolder: self.favoritesFolder) + guard let bookmarkMO = bookmarkFetchRequestResults?.first else { + assertionFailure("LocalBookmarkStore: Failed to get BookmarkEntity from the context") + throw BookmarkStoreError.missingEntity + } - do { - try self.context.save(onErrorFire: .bookmarksSaveFailed) - } catch { - self.context.rollback() - assertionFailure("LocalBookmarkStore: Saving of context failed") - } + bookmarkMO.update(with: bookmark, favoritesFolder: self.favoritesRoot(in: context)) + }) + + } catch { + let error = error as NSError + commonOnSaveErrorHandler(error) } } func update(folder: BookmarkFolder) { - context.performAndWait { [weak self] in - guard let self = self else { - return - } - - let folderFetchRequest = BaseBookmarkEntity.singleEntity(with: folder.id) - let folderFetchRequestResults = try? self.context.fetch(folderFetchRequest) - guard let bookmarkFolderMO = folderFetchRequestResults?.first else { - assertionFailure("LocalBookmarkStore: Failed to get BookmarkEntity from the context") - return - } + do { + _ = try applyChangesAndSave(changes: { context in + let folderFetchRequest = BaseBookmarkEntity.singleEntity(with: folder.id) + let folderFetchRequestResults = try? context.fetch(folderFetchRequest) - bookmarkFolderMO.update(with: folder) + guard let bookmarkFolderMO = folderFetchRequestResults?.first else { + assertionFailure("LocalBookmarkStore: Failed to get BookmarkEntity from the context") + throw BookmarkStoreError.missingEntity + } - do { - try self.context.save(onErrorFire: .bookmarksSaveFailed) - } catch { - self.context.rollback() - assertionFailure("LocalBookmarkStore: Saving of context failed") - } + bookmarkFolderMO.update(with: folder) + }) + } catch { + let error = error as NSError + commonOnSaveErrorHandler(error) } } func add(objectsWithUUIDs uuids: [String], to parent: BookmarkFolder?, completion: @escaping (Error?) -> Void) { - context.perform { [weak self] in + + applyChangesAndSave(changes: { [weak self] context in guard let self = self else { - completion(nil) - return + throw BookmarkStoreError.storeDeallocated } let bookmarksFetchRequest = BaseBookmarkEntity.entities(with: uuids) bookmarksFetchRequest.returnsObjectsAsFaults = false - let bookmarksResults = try? self.context.fetch(bookmarksFetchRequest) + let bookmarksResults = try? context.fetch(bookmarksFetchRequest) guard let bookmarkManagedObjects = bookmarksResults else { assertionFailure("\(#file): Failed to get BookmarkEntity from the context") - completion(nil) - return + throw BookmarkStoreError.missingEntity } if let parentUUID = parent?.id { let parentFetchRequest = BaseBookmarkEntity.singleEntity(with: parentUUID) - let parentFetchRequestResults = try? self.context.fetch(parentFetchRequest) + let parentFetchRequestResults = try? context.fetch(parentFetchRequest) guard let parentManagedObject = parentFetchRequestResults?.first, parentManagedObject.isFolder else { assertionFailure("\(#file): Failed to get BookmarkEntity from the context") - completion(nil) - return + throw BookmarkStoreError.missingEntity } parentManagedObject.addToChildren(NSOrderedSet(array: bookmarkManagedObjects)) } else { + let root = self.bookmarksRoot(in: context) for bookmarkManagedObject in bookmarkManagedObjects { - bookmarkManagedObject.parent = self.rootLevelFolder + bookmarkManagedObject.parent = root } } - - do { - try self.context.save(onErrorFire: .bookmarksSaveFailed) - } catch { - self.context.rollback() - assertionFailure("\(#file): Saving of context failed: \(error)") - DispatchQueue.main.async { completion(error) } - return - } - + }, onError: { [weak self] error in + self?.commonOnSaveErrorHandler(error) + DispatchQueue.main.async { completion(error) } + }, onDidSave: { DispatchQueue.main.async { completion(nil) } - } + }) } func update(objectsWithUUIDs uuids: [String], update: @escaping (BaseBookmarkEntity) -> Void, completion: @escaping (Error?) -> Void) { - context.perform { [weak self] in + + applyChangesAndSave(changes: { [weak self] context in guard let self = self else { - completion(nil) - return + throw BookmarkStoreError.storeDeallocated } let bookmarksFetchRequest = BaseBookmarkEntity.entities(with: uuids) bookmarksFetchRequest.returnsObjectsAsFaults = false - let bookmarksResults = try? self.context.fetch(bookmarksFetchRequest) + let bookmarksResults = try? context.fetch(bookmarksFetchRequest) guard let bookmarkManagedObjects = bookmarksResults else { assertionFailure("\(#file): Failed to get BookmarkEntity from the context") - completion(nil) - return + throw BookmarkStoreError.missingEntity } + let favoritesRoot = self.favoritesRoot(in: context) bookmarkManagedObjects.forEach { managedObject in if let entity = BaseBookmarkEntity.from(managedObject: managedObject, parentFolderUUID: nil) { update(entity) - managedObject.update(with: entity, favoritesFolder: self.favoritesFolder) + managedObject.update(with: entity, favoritesFolder: favoritesRoot) } } - - do { - try self.context.save(onErrorFire: .bookmarksSaveFailed) - } catch { - self.context.rollback() - assertionFailure("\(#file): Saving of context failed") - DispatchQueue.main.async { completion(error) } - return - } - + }, onError: { [weak self] error in + self?.commonOnSaveErrorHandler(error) + DispatchQueue.main.async { completion(error) } + }, onDidSave: { DispatchQueue.main.async { completion(nil) } - } + }) } // MARK: - Folders func save(folder: BookmarkFolder, parent: BookmarkFolder?, completion: @escaping (Bool, Error?) -> Void) { - context.perform { [weak self] in + + applyChangesAndSave(changes: { [weak self] context in guard let self = self else { - DispatchQueue.main.async { completion(false, BookmarkStoreError.storeDeallocated) } - return + throw BookmarkStoreError.storeDeallocated } let parentEntity: BookmarkEntity if let parent = parent, - let parentFetchRequestResult = try? self.context.fetch(BaseBookmarkEntity.singleEntity(with: parent.id)).first { + let parentFetchRequestResult = try? context.fetch(BaseBookmarkEntity.singleEntity(with: parent.id)).first { parentEntity = parentFetchRequestResult - } else if let root = BookmarkUtils.fetchRootFolder(self.context) { + } else if let root = self.bookmarksRoot(in: context) { parentEntity = root } else { Pixel.fire(.debug(event: .missingParent)) - DispatchQueue.main.async { completion(false, BookmarkStoreError.missingParent) } - return + throw BookmarkStoreError.missingParent } let bookmarkMO = BookmarkEntity.makeFolder(title: folder.title, parent: parentEntity, - context: self.context) + context: context) bookmarkMO.uuid = folder.id - - do { - try self.context.save(onErrorFire: .bookmarksSaveFailed) - } catch { - self.context.rollback() - // Only throw this assertion when running in debug and when unit tests are not running. - if !NSApp.isRunningUnitTests { - assertionFailure("LocalBookmarkStore: Saving of context failed") - } - - DispatchQueue.main.async { completion(false, error) } - return - } - + }, onError: { [weak self] error in + self?.commonOnSaveErrorHandler(error) + DispatchQueue.main.async { completion(false, error) } + }, onDidSave: { DispatchQueue.main.async { completion(true, nil) } - } + }) } func canMoveObjectWithUUID(objectUUID uuid: String, to parent: BookmarkFolder) -> Bool { @@ -389,6 +486,7 @@ final class LocalBookmarkStore: BookmarkStore { // Assume true by default – the database validations will serve as a final check before any invalid state makes it into the database. var canMoveObject = true + let context = makeContext() context.performAndWait { [weak self] in guard let self = self else { assertionFailure("Couldn't get strong self") @@ -396,10 +494,10 @@ final class LocalBookmarkStore: BookmarkStore { } let folderToMoveFetchRequest = BaseBookmarkEntity.singleEntity(with: uuid) - let folderToMoveFetchRequestResults = try? self.context.fetch(folderToMoveFetchRequest) + let folderToMoveFetchRequestResults = try? context.fetch(folderToMoveFetchRequest) let parentFolderFetchRequest = BaseBookmarkEntity.singleEntity(with: parent.id) - let parentFolderFetchRequestResults = try? self.context.fetch(parentFolderFetchRequest) + let parentFolderFetchRequestResults = try? context.fetch(parentFolderFetchRequest) guard let folderToMove = folderToMoveFetchRequestResults?.first, let parentFolder = parentFolderFetchRequestResults?.first, @@ -425,24 +523,21 @@ final class LocalBookmarkStore: BookmarkStore { } func move(objectUUIDs: [String], toIndex index: Int?, withinParentFolder type: ParentFolderType, completion: @escaping (Error?) -> Void) { - context.perform { [weak self] in + + applyChangesAndSave(changes: { [weak self] context in guard let self = self else { - assertionFailure("Couldn't get strong self") - completion(nil) - return + throw BookmarkStoreError.storeDeallocated } - guard let rootFolder = self.rootLevelFolder else { - assertionFailure("\(#file): Failed to get root level folder") - completion(nil) - return + guard let rootFolder = self.bookmarksRoot(in: context) else { + throw BookmarkStoreError.missingRoot } // Guarantee that bookmarks are fetched in the same order as the UUIDs. In the future, this should fetch all objects at once with a // batch fetch request and have them sorted in the correct order. let bookmarkManagedObjects: [BookmarkEntity] = objectUUIDs.compactMap { uuid in let entityFetchRequest = BaseBookmarkEntity.singleEntity(with: uuid) - return (try? self.context.fetch(entityFetchRequest))?.first + return (try? context.fetch(entityFetchRequest))?.first } let newParentFolder: BookmarkEntity @@ -452,16 +547,10 @@ final class LocalBookmarkStore: BookmarkStore { case .parent(let newParentUUID): let bookmarksFetchRequest = BaseBookmarkEntity.singleEntity(with: newParentUUID) - do { - if let fetchedParent = try self.context.fetch(bookmarksFetchRequest).first, fetchedParent.isFolder { - newParentFolder = fetchedParent - } else { - completion(nil) - return - } - } catch { - completion(error) - return + if let fetchedParent = try context.fetch(bookmarksFetchRequest).first, fetchedParent.isFolder { + newParentFolder = fetchedParent + } else { + throw BookmarkStoreError.missingEntity } } @@ -473,18 +562,12 @@ final class LocalBookmarkStore: BookmarkStore { newParentFolder.addToChildren(bookmarkManagedObject) } } - - do { - try self.context.save(onErrorFire: .bookmarksSaveFailed) - } catch { - self.context.rollback() - assertionFailure("\(#file): Saving of context failed") - DispatchQueue.main.async { completion(error) } - return - } - + }, onError: { [weak self] error in + self?.commonOnSaveErrorHandler(error) + DispatchQueue.main.async { completion(error) } + }, onDidSave: { DispatchQueue.main.async { completion(nil) } - } + }) } private func move(entities bookmarkManagedObjects: [BookmarkEntity], to index: Int, within newParentFolder: BookmarkEntity) { @@ -516,24 +599,21 @@ final class LocalBookmarkStore: BookmarkStore { } func moveFavorites(with objectUUIDs: [String], toIndex index: Int?, completion: @escaping (Error?) -> Void) { - context.perform { [weak self] in + + applyChangesAndSave(changes: { [weak self] context in guard let self = self else { - assertionFailure("Couldn't get strong self") - completion(nil) - return + throw BookmarkStoreError.storeDeallocated } - guard let favoritesFolder = self.favoritesFolder else { - assertionFailure("\(#file): Failed to get favorites folder") - completion(nil) - return + guard let favoritesFolder = self.favoritesRoot(in: context) else { + throw BookmarkStoreError.missingFavoritesRoot } // Guarantee that bookmarks are fetched in the same order as the UUIDs. In the future, this should fetch all objects at once with a // batch fetch request and have them sorted in the correct order. let bookmarkManagedObjects: [BookmarkEntity] = objectUUIDs.compactMap { uuid in let entityFetchRequest = BaseBookmarkEntity.favorite(with: uuid) - return (try? self.context.fetch(entityFetchRequest))?.first + return (try? context.fetch(entityFetchRequest))?.first } if let index = index, index < (favoritesFolder.favorites?.count ?? 0) { @@ -563,18 +643,13 @@ final class LocalBookmarkStore: BookmarkStore { bookmarkManagedObject.addToFavorites(favoritesRoot: favoritesFolder) } } - - do { - try self.context.save(onErrorFire: .bookmarksSaveFailed) - } catch { - self.context.rollback() - assertionFailure("\(#file): Saving of context failed") - DispatchQueue.main.async { completion(error) } - return - } - + }, onError: { [weak self] error in + self?.commonOnSaveErrorHandler(error) + DispatchQueue.main.async { completion(error) } + }, onDidSave: { DispatchQueue.main.async { completion(nil) } - } + }) + } // MARK: - Import @@ -591,53 +666,48 @@ final class LocalBookmarkStore: BookmarkStore { func importBookmarks(_ bookmarks: ImportedBookmarks, source: BookmarkImportSource) -> BookmarkImportResult { var total = BookmarkImportResult(successful: 0, duplicates: 0, failed: 0) - context.performAndWait { - do { - let bookmarkCountBeforeImport = try context.count(for: Bookmark.bookmarksFetchRequest()) - let allFolders = try context.fetch(BookmarkFolder.bookmarkFoldersFetchRequest()) - - total += createEntitiesFromBookmarks(allFolders: allFolders, bookmarks: bookmarks, importSourceName: source.importSourceName) + do { + let context = makeContext() + var bookmarkCountBeforeImport = 0 + context.performAndWait { + bookmarkCountBeforeImport = (try? context.count(for: Bookmark.bookmarksFetchRequest())) ?? 0 + } - try self.context.save(onErrorFire: .bookmarksSaveFailedOnImport) - let bookmarkCountAfterImport = try context.count(for: Bookmark.bookmarksFetchRequest()) + try applyChangesAndSave { context in + let allFolders = try context.fetch(BookmarkFolder.bookmarkFoldersFetchRequest()) + total += createEntitiesFromBookmarks(allFolders: allFolders, + bookmarks: bookmarks, + importSourceName: source.importSourceName, + in: context) + } + context.performAndWait { + let bookmarkCountAfterImport = (try? context.count(for: Bookmark.bookmarksFetchRequest())) ?? 0 total.successful = bookmarkCountAfterImport - bookmarkCountBeforeImport - } catch { - self.context.rollback() - os_log("Failed to import bookmarks, with error: %s", log: .dataImportExport, type: .error, error.localizedDescription) - - // Only throw this assertion when running in debug and when unit tests are not running. - if !NSApp.isRunningUnitTests { - assertionFailure("LocalBookmarkStore: Saving of context failed, error: \(error.localizedDescription)") - } } - } - - return total - } - private func createEntitiesFromDDGWebKitBookmarks(allFolders: [BookmarkEntity], bookmarks: ImportedBookmarks) -> BookmarkImportResult { + } catch { + os_log("Failed to import bookmarks, with error: %s", log: .dataImportExport, type: .error, error.localizedDescription) - var total = BookmarkImportResult(successful: 0, duplicates: 0, failed: 0) + let error = error as NSError + let processedErrors = CoreDataErrorsParser.parse(error: error) - if let otherBookmarks = bookmarks.topLevelFolders.otherBookmarks.children, - let rootFolder = rootLevelFolder { - let result = recursivelyCreateEntities(from: otherBookmarks, - parent: rootFolder, - in: self.context) - - total += result + if !NSApp.isRunningUnitTests { + Pixel.fire(.debug(event: .bookmarksSaveFailedOnImport, error: error), + withAdditionalParameters: processedErrors.errorPixelParameters) + assertionFailure("LocalBookmarkStore: Saving of context failed, error: \(error.localizedDescription)") + } } return total - } private func createEntitiesFromBookmarks(allFolders: [BookmarkEntity], bookmarks: ImportedBookmarks, - importSourceName: String) -> BookmarkImportResult { + importSourceName: String, + in context: NSManagedObjectContext) -> BookmarkImportResult { - guard let root = rootLevelFolder else { + guard let root = bookmarksRoot(in: context) else { return .init(successful: 0, duplicates: 0, failed: bookmarks.numberOfBookmarks) @@ -658,7 +728,7 @@ final class LocalBookmarkStore: BookmarkStore { let result = recursivelyCreateEntities(from: bookmarksBar, parent: parent, markBookmarksAsFavorite: makeFavorties, - in: self.context) + in: context) total += result } @@ -667,7 +737,7 @@ final class LocalBookmarkStore: BookmarkStore { let result = recursivelyCreateEntities(from: otherBookmarks, parent: parent, markBookmarksAsFavorite: false, - in: self.context) + in: context) total += result } @@ -701,7 +771,7 @@ final class LocalBookmarkStore: BookmarkStore { } // Bookmarks from the bookmarks bar are imported as favorites - if let favoritesRoot = favoritesFolder, + if let favoritesRoot = favoritesRoot(in: context), bookmarkOrFolder.isDDGFavorite || (!bookmarkOrFolder.isFolder && markBookmarksAsFavorite == true) { bookmarkManagedObject.addToFavorites(favoritesRoot: favoritesRoot) } @@ -735,32 +805,31 @@ final class LocalBookmarkStore: BookmarkStore { /// There is a rare issue where bookmark managed objects can end up in the database with an invalid state, that is that they are missing their title value despite being non-optional. /// They appear to be disjoint from a user's actual bookmarks data, so this function removes them. private func removeInvalidBookmarkEntities() { - context.performAndWait { + + applyChangesAndSave { context in let entitiesFetchRequest = BookmarkEntity.fetchRequest() - do { - let entities = try self.context.fetch(entitiesFetchRequest) + let entities = try context.fetch(entitiesFetchRequest) + var deletedEntityCount = 0 - var deletedEntityCount = 0 + for entity in entities where entity.isInvalid { + context.delete(entity) + deletedEntityCount += 1 + } - for entity in entities where entity.isInvalid { - self.context.delete(entity) - deletedEntityCount += 1 - } + if deletedEntityCount > 0 { + Pixel.fire(.debug(event: .removedInvalidBookmarkManagedObjects)) + } + } onError: { [weak self] error in + self?.commonOnSaveErrorHandler(error) - if deletedEntityCount > 0 { - Pixel.fire(.debug(event: .removedInvalidBookmarkManagedObjects)) - } + os_log("Failed to remove invalid bookmark entities", type: .error) + } onDidSave: {} - try self.context.save(onErrorFire: .bookmarksSaveFailed) - } catch { - self.context.rollback() - os_log("Failed to remove invalid bookmark entities", type: .error) - } - } } func resetBookmarks() { + let context = makeContext() context.performAndWait { let fetchRequest: NSFetchRequest = NSFetchRequest(entityName: BookmarkEntity.className()) let deleteRequest = NSBatchDeleteRequest(fetchRequest: fetchRequest) @@ -772,7 +841,7 @@ final class LocalBookmarkStore: BookmarkStore { } } - sharedInitialization() + cacheReadOnlyTopLevelBookmarksFolders() } // MARK: - Concurrency @@ -848,6 +917,31 @@ final class LocalBookmarkStore: BookmarkStore { } +extension LocalBookmarkStore.BookmarkStoreError: CustomNSError { + + var errorCode: Int { + switch self { + case .storeDeallocated: return 1 + case .noObjectId: return 2 + case .badObjectId: return 3 + case .asyncFetchFailed: return 4 + case .missingParent: return 5 + case .missingEntity: return 6 + case .missingRoot: return 7 + case .missingFavoritesRoot: return 8 + case .saveLoopError: return 9 + } + } + + var errorUserInfo: [String: Any] { + guard case .saveLoopError(let error) = self, let error else { + return [:] + } + + return [NSUnderlyingErrorKey: error as NSError] + } +} + fileprivate extension BookmarkEntity { var isInvalid: Bool { diff --git a/UnitTests/Bookmarks/Services/LocalBookmarkStoreSavingTests.swift b/UnitTests/Bookmarks/Services/LocalBookmarkStoreSavingTests.swift new file mode 100644 index 0000000000..e33f5fea00 --- /dev/null +++ b/UnitTests/Bookmarks/Services/LocalBookmarkStoreSavingTests.swift @@ -0,0 +1,337 @@ +// +// LocalBookmarkStoreSavingTests.swift +// +// Copyright © 2023 DuckDuckGo. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import Foundation +import Bookmarks +import XCTest +import CoreData +@testable import DuckDuckGo_Privacy_Browser + +final class LocalBookmarkStoreSavingTests: XCTestCase { + + enum LocalError: Error { + case example + } + + // MARK: Save/Delete + + let container = CoreData.bookmarkContainer() + var store: LocalBookmarkStore! + + override func setUp() { + super.setUp() + + BookmarkUtils.prepareFoldersStructure(in: container.viewContext) + + do { + try container.viewContext.save() + } catch { + XCTFail("Could not prepare Bookmarks Structure") + } + + store = LocalBookmarkStore { + self.container.newBackgroundContext() + } + } + + func testWhenThereIsNoErrorThenDataIsSaved() throws { + let otherContext = container.newBackgroundContext() + + try store.applyChangesAndSave { context in + let root = BookmarkUtils.fetchRootFolder(context) + _ = BookmarkEntity.makeBookmark(title: "T", url: "h", parent: root!, context: context) + } + + otherContext.performAndWait { + let root = BookmarkUtils.fetchRootFolder(otherContext) + XCTAssertEqual(root?.childrenArray.first?.title, "T") + XCTAssertEqual(root?.childrenArray.first?.url, "h") + } + } + + func testWhenThereIsNoErrorThenDataIsSaved_Closures() throws { + let otherContext = container.newBackgroundContext() + + let expectation = expectation(description: "Did save") + + store.applyChangesAndSave { context in + let root = BookmarkUtils.fetchRootFolder(context) + _ = BookmarkEntity.makeBookmark(title: "T", url: "h", parent: root!, context: context) + } onError: { _ in + XCTFail("Error not expected") + } onDidSave: { + expectation.fulfill() + } + + wait(for: [expectation], timeout: 5) + + otherContext.performAndWait { + let root = BookmarkUtils.fetchRootFolder(otherContext) + XCTAssertEqual(root?.childrenArray.first?.title, "T") + XCTAssertEqual(root?.childrenArray.first?.url, "h") + } + } + + func testWhenThereIsExplicitErrorThenOnErrorIsCalled() { + let otherContext = container.newBackgroundContext() + + do { + try store.applyChangesAndSave { context in + let root = BookmarkUtils.fetchRootFolder(context) + _ = BookmarkEntity.makeBookmark(title: "T", url: "h", parent: root!, context: context) + + throw LocalError.example + } + XCTFail("Exception should be thrown") + } catch { + XCTAssertEqual(error as? LocalError, .example) + } + + otherContext.performAndWait { + let root = BookmarkUtils.fetchRootFolder(otherContext) + XCTAssert(root!.childrenArray.isEmpty) + } + } + + func testWhenThereIsExplicitErrorThenOnErrorIsCalled_Closures() { + let otherContext = container.newBackgroundContext() + + let expectation = expectation(description: "OnError") + + store.applyChangesAndSave { context in + let root = BookmarkUtils.fetchRootFolder(context) + _ = BookmarkEntity.makeBookmark(title: "T", url: "h", parent: root!, context: context) + + throw LocalError.example + } onError: { error in + expectation.fulfill() + XCTAssertEqual(error as? LocalError, .example) + } onDidSave: { + XCTFail("Should not save") + } + + wait(for: [expectation], timeout: 5) + + otherContext.performAndWait { + let root = BookmarkUtils.fetchRootFolder(otherContext) + XCTAssert(root!.childrenArray.isEmpty) + } + } + + func testWhenThereIsSaveErrorThenOnErrorIsCalled() { + let otherContext = container.newBackgroundContext() + + do { + try store.applyChangesAndSave { context in + let root = BookmarkUtils.fetchRootFolder(context) + let folder = BookmarkEntity.makeFolder(title: "Folder", parent: root!, context: context) + folder.url = "incorrect value" + } + XCTFail("Exception should be thrown") + } catch { + XCTAssertEqual(error as? BookmarkEntity.Error, BookmarkEntity.Error.folderHasURL) + } + + otherContext.performAndWait { + let root = BookmarkUtils.fetchRootFolder(otherContext) + XCTAssert(root!.childrenArray.isEmpty) + } + } + + func testWhenThereIsSaveErrorThenOnErrorIsCalled_Closures() { + let otherContext = container.newBackgroundContext() + + let expectation = expectation(description: "OnError") + + store.applyChangesAndSave { context in + let root = BookmarkUtils.fetchRootFolder(context) + let folder = BookmarkEntity.makeFolder(title: "Folder", parent: root!, context: context) + folder.url = "incorrect value" + } onError: { error in + expectation.fulfill() + XCTAssertEqual(error as? BookmarkEntity.Error, BookmarkEntity.Error.folderHasURL) + } onDidSave: { + XCTFail("Should not save") + } + + wait(for: [expectation], timeout: 5) + + otherContext.performAndWait { + let root = BookmarkUtils.fetchRootFolder(otherContext) + XCTAssert(root!.childrenArray.isEmpty) + } + } + + func testWhenThereIsMergeErrorThenSaveRetries() { + + let otherContext = container.newBackgroundContext() + + do { + try store.applyChangesAndSave { context in + let root = BookmarkUtils.fetchRootFolder(context) + _ = BookmarkEntity.makeBookmark(title: "T", url: "h", parent: root!, context: context) + + otherContext.performAndWait { + let root = BookmarkUtils.fetchRootFolder(otherContext) + + // Only store on first pass + guard root?.childrenArray.isEmpty ?? false else { return } + + _ = BookmarkEntity.makeBookmark(title: "Inner", url: "i", parent: root!, context: otherContext) + do { + try otherContext.save() + } catch { + XCTFail("Could not save inner object") + } + } + } + } catch { + XCTFail("Exception should not be thrown") + } + + otherContext.performAndWait { + otherContext.reset() + let root = BookmarkUtils.fetchRootFolder(otherContext) + let children = root?.childrenArray ?? [] + + XCTAssertEqual(children.count, 2) + XCTAssertEqual(Set(children.map { $0.title }), ["T", "Inner"]) + } + } + + func testWhenThereIsMergeErrorThenSaveRetries_Closures() { + + let otherContext = container.newBackgroundContext() + + let expectation = expectation(description: "On DidSave") + + store.applyChangesAndSave { context in + let root = BookmarkUtils.fetchRootFolder(context) + _ = BookmarkEntity.makeBookmark(title: "T", url: "h", parent: root!, context: context) + + otherContext.performAndWait { + let root = BookmarkUtils.fetchRootFolder(otherContext) + + // Only store on first pass + guard root?.childrenArray.isEmpty ?? false else { return } + + _ = BookmarkEntity.makeBookmark(title: "Inner", url: "i", parent: root!, context: otherContext) + do { + try otherContext.save() + } catch { + XCTFail("Could not save inner object") + } + } + } onError: { _ in + XCTFail("No error expected") + } onDidSave: { + expectation.fulfill() + } + + wait(for: [expectation], timeout: 5) + + otherContext.performAndWait { + otherContext.reset() + let root = BookmarkUtils.fetchRootFolder(otherContext) + let children = root?.childrenArray ?? [] + + XCTAssertEqual(children.count, 2) + XCTAssertEqual(Set(children.map { $0.title }), ["T", "Inner"]) + } + } + + func testWhenThereIsRecurringMergeErrorThenOnErrorIsCalled() { + let otherContext = container.newBackgroundContext() + + do { + try store.applyChangesAndSave { context in + let root = BookmarkUtils.fetchRootFolder(context) + _ = BookmarkEntity.makeBookmark(title: "T", url: "h", parent: root!, context: context) + + otherContext.performAndWait { + let root = BookmarkUtils.fetchRootFolder(otherContext) + _ = BookmarkEntity.makeBookmark(title: "Inner", url: "i", parent: root!, context: otherContext) + do { + try otherContext.save() + } catch { + XCTFail("Could not save inner object") + } + } + } + XCTFail("Should trow an error") + } catch { + if case LocalBookmarkStore.BookmarkStoreError.saveLoopError(let wrappedError) = error, let wrappedError { + XCTAssertEqual((wrappedError as NSError).code, NSManagedObjectMergeError) + } else { + XCTFail("Loop Error expected") + } + } + + otherContext.performAndWait { + otherContext.reset() + let root = BookmarkUtils.fetchRootFolder(otherContext) + let children = root?.childrenArray ?? [] + + XCTAssertEqual(children.count, 4) + XCTAssertEqual(Set(children.map { $0.title }), ["Inner"]) + } + } + + func testWhenThereIsRecurringMergeErrorThenOnErrorIsCalled_Closures() { + let otherContext = container.newBackgroundContext() + + let expectation = expectation(description: "OnError") + + store.applyChangesAndSave { context in + let root = BookmarkUtils.fetchRootFolder(context) + _ = BookmarkEntity.makeBookmark(title: "T", url: "h", parent: root!, context: context) + + otherContext.performAndWait { + let root = BookmarkUtils.fetchRootFolder(otherContext) + _ = BookmarkEntity.makeBookmark(title: "Inner", url: "i", parent: root!, context: otherContext) + do { + try otherContext.save() + } catch { + XCTFail("Could not save inner object") + } + } + } onError: { error in + expectation.fulfill() + + if case LocalBookmarkStore.BookmarkStoreError.saveLoopError(let wrappedError) = error, let wrappedError { + XCTAssertEqual((wrappedError as NSError).code, NSManagedObjectMergeError) + } else { + XCTFail("Loop Error expected") + } + } onDidSave: { + XCTFail("Did save should not be called") + } + + wait(for: [expectation], timeout: 5) + + otherContext.performAndWait { + otherContext.reset() + let root = BookmarkUtils.fetchRootFolder(otherContext) + let children = root?.childrenArray ?? [] + + XCTAssertEqual(children.count, 4) + XCTAssertEqual(Set(children.map { $0.title }), ["Inner"]) + } + } + +} diff --git a/UnitTests/Bookmarks/Services/LocalBookmarkStoreTests.swift b/UnitTests/Bookmarks/Services/LocalBookmarkStoreTests.swift index c3abb7246f..d1745e3be0 100644 --- a/UnitTests/Bookmarks/Services/LocalBookmarkStoreTests.swift +++ b/UnitTests/Bookmarks/Services/LocalBookmarkStoreTests.swift @@ -21,6 +21,15 @@ import Bookmarks import XCTest @testable import DuckDuckGo_Privacy_Browser +extension LocalBookmarkStore { + + convenience init(context: NSManagedObjectContext) { + self.init { + context + } + } +} + final class LocalBookmarkStoreTests: XCTestCase { // MARK: Save/Delete @@ -31,6 +40,11 @@ final class LocalBookmarkStoreTests: XCTestCase { super.setUp() BookmarkUtils.prepareFoldersStructure(in: container.viewContext) + do { + try container.viewContext.save() + } catch { + XCTFail("Could not prepare Bookmarks Structure") + } } func testWhenBookmarkIsSaved_ThenItMustBeLoadedFromStore() {