diff --git a/Sources/Bookmarks/BookmarkUtils.swift b/Sources/Bookmarks/BookmarkUtils.swift index 9b3a63410..33ab4ad59 100644 --- a/Sources/Bookmarks/BookmarkUtils.swift +++ b/Sources/Bookmarks/BookmarkUtils.swift @@ -25,9 +25,11 @@ public struct BookmarkUtils { let request = BookmarkEntity.fetchRequest() request.predicate = NSPredicate(format: "%K == %@", #keyPath(BookmarkEntity.uuid), BookmarkEntity.Constants.rootFolderID) request.returnsObjectsAsFaults = false - request.fetchLimit = 1 - return try? context.fetch(request).first + let result = (try? context.fetch(request)) ?? [] + + // We cannot use simply sort descriptor as this is to-many on both sides of a relationship. + return result.sorted(by: { ($0.children?.count ?? 0) > ($1.children?.count ?? 0) }).first } public static func fetchFavoritesFolders(for displayMode: FavoritesDisplayMode, in context: NSManagedObjectContext) -> [BookmarkEntity] { @@ -40,9 +42,11 @@ public struct BookmarkUtils { let request = BookmarkEntity.fetchRequest() request.predicate = NSPredicate(format: "%K == %@", #keyPath(BookmarkEntity.uuid), uuid) request.returnsObjectsAsFaults = false - request.fetchLimit = 1 - return try? context.fetch(request).first + let result = (try? context.fetch(request)) ?? [] + + // We cannot use simply sort descriptor as this is to-many on both sides of a relationship. + return result.sorted(by: { ($0.favorites?.count ?? 0) > ($1.favorites?.count ?? 0) }).first } public static func fetchFavoritesFolders(withUUIDs uuids: Set, in context: NSManagedObjectContext) -> [BookmarkEntity] { @@ -51,9 +55,18 @@ public struct BookmarkUtils { let request = BookmarkEntity.fetchRequest() request.predicate = NSPredicate(format: "%K in %@", #keyPath(BookmarkEntity.uuid), uuids) request.returnsObjectsAsFaults = false - request.fetchLimit = uuids.count - return (try? context.fetch(request)) ?? [] + var objects = (try? context.fetch(request)) ?? [] + objects.sort(by: { ($0.favorites?.count ?? 0) > ($1.favorites?.count ?? 0) }) + + var result = [BookmarkEntity]() + for uuid in uuids { + if let entity = objects.first(where: { $0.uuid == uuid }) { + result.append(entity) + } + } + + return result } public static func fetchOrphanedEntities(_ context: NSManagedObjectContext) -> [BookmarkEntity] { @@ -227,15 +240,22 @@ public struct BookmarkUtils { extension BookmarkUtils { - public static func prepareLegacyFoldersStructure(in context: NSManagedObjectContext) { + public static func prepareLegacyFoldersStructure(in context: NSManagedObjectContext) throws { - if fetchRootFolder(context) == nil { - insertRootFolder(uuid: BookmarkEntity.Constants.rootFolderID, into: context) - } + func prepareRootFolder(uuid: String) throws { + let request = BookmarkEntity.fetchRequest() + request.predicate = NSPredicate(format: "%K == %@", #keyPath(BookmarkEntity.uuid), uuid) + request.returnsObjectsAsFaults = false + request.fetchLimit = 1 - if fetchLegacyFavoritesFolder(context) == nil { - insertRootFolder(uuid: legacyFavoritesFolderID, into: context) + let root = try context.fetch(request).first + if root == nil { + insertRootFolder(uuid: uuid, into: context) + } } + + try prepareRootFolder(uuid: BookmarkEntity.Constants.rootFolderID) + try prepareRootFolder(uuid: legacyFavoritesFolderID) } public static func fetchLegacyFavoritesFolder(_ context: NSManagedObjectContext) -> BookmarkEntity? { diff --git a/Sources/Bookmarks/Migrations/BookmarkFormFactorFavoritesMigration.swift b/Sources/Bookmarks/Migrations/BookmarkFormFactorFavoritesMigration.swift index f8d170d13..40689988d 100644 --- a/Sources/Bookmarks/Migrations/BookmarkFormFactorFavoritesMigration.swift +++ b/Sources/Bookmarks/Migrations/BookmarkFormFactorFavoritesMigration.swift @@ -21,15 +21,18 @@ import CoreData import Persistence import Common -public class BookmarkFormFactorFavoritesMigration { +public protocol BookmarkFormFactorFavoritesMigrating { - public enum MigrationErrors { - case couldNotLoadDatabase - } + func getFavoritesOrderFromPreV4Model(dbContainerLocation: URL, + dbFileURL: URL) throws -> [String]? +} + +public class BookmarkFormFactorFavoritesMigration: BookmarkFormFactorFavoritesMigrating { + + public init() {} - public static func getFavoritesOrderFromPreV4Model(dbContainerLocation: URL, - dbFileURL: URL, - errorEvents: EventMapping? = nil) -> [String]? { + public func getFavoritesOrderFromPreV4Model(dbContainerLocation: URL, + dbFileURL: URL) throws -> [String]? { guard let metadata = try? NSPersistentStoreCoordinator.metadataForPersistentStore(ofType: NSSQLiteStoreType, at: dbFileURL), let latestModel = CoreDataDatabase.loadModel(from: bundle, named: "BookmarksModel"), @@ -61,9 +64,10 @@ public class BookmarkFormFactorFavoritesMigration { var oldFavoritesOrder: [String]? + var loadError: Error? oldDB.loadStore { context, error in guard let context = context else { - errorEvents?.fire(.couldNotLoadDatabase, error: error) + loadError = error return } @@ -71,6 +75,11 @@ public class BookmarkFormFactorFavoritesMigration { let orderedFavorites = favs?.favorites?.array as? [BookmarkEntity] ?? [] oldFavoritesOrder = orderedFavorites.compactMap { $0.uuid } } + + if let loadError { + throw loadError + } + return oldFavoritesOrder } diff --git a/Sources/BookmarksTestDBBuilder/BookmarksTestDBBuilder.swift b/Sources/BookmarksTestDBBuilder/BookmarksTestDBBuilder.swift index e468960c9..4d28b8744 100644 --- a/Sources/BookmarksTestDBBuilder/BookmarksTestDBBuilder.swift +++ b/Sources/BookmarksTestDBBuilder/BookmarksTestDBBuilder.swift @@ -229,7 +229,7 @@ public struct BookmarkTree { @discardableResult public func createEntitiesForCheckingModifiedAt(in context: NSManagedObjectContext) -> (BookmarkEntity, [BookmarkEntity]) { - BookmarkUtils.prepareLegacyFoldersStructure(in: context) + try? BookmarkUtils.prepareLegacyFoldersStructure(in: context) let rootFolder = BookmarkUtils.fetchRootFolder(context)! rootFolder.modifiedAt = modifiedAt diff --git a/Sources/Persistence/CoreDataDatabase.swift b/Sources/Persistence/CoreDataDatabase.swift index f29ba2d5d..40005acda 100644 --- a/Sources/Persistence/CoreDataDatabase.swift +++ b/Sources/Persistence/CoreDataDatabase.swift @@ -20,12 +20,25 @@ import Foundation import CoreData import Common -public protocol ManagedObjectContextFactory { +public protocol CoreDataStoring { + + var isDatabaseFileInitialized: Bool { get } + var model: NSManagedObjectModel { get } + var coordinator: NSPersistentStoreCoordinator { get } + + func loadStore(completion: @escaping (NSManagedObjectContext?, Swift.Error?) -> Void) func makeContext(concurrencyType: NSManagedObjectContextConcurrencyType, name: String?) -> NSManagedObjectContext } -public class CoreDataDatabase: ManagedObjectContextFactory { +public extension CoreDataStoring { + + func makeContext(concurrencyType: NSManagedObjectContextConcurrencyType) -> NSManagedObjectContext { + makeContext(concurrencyType: concurrencyType, name: nil) + } +} + +public class CoreDataDatabase: CoreDataStoring { public enum Error: Swift.Error { case containerLocationCouldNotBePrepared(underlyingError: Swift.Error) @@ -135,7 +148,7 @@ public class CoreDataDatabase: ManagedObjectContextFactory { } } - public func makeContext(concurrencyType: NSManagedObjectContextConcurrencyType, name: String? = nil) -> NSManagedObjectContext { + public func makeContext(concurrencyType: NSManagedObjectContextConcurrencyType, name: String?) -> NSManagedObjectContext { RunLoop.current.run(until: storeLoadedCondition) let context = NSManagedObjectContext(concurrencyType: concurrencyType) diff --git a/Tests/BookmarksTests/BookmarkMigrationTests.swift b/Tests/BookmarksTests/BookmarkMigrationTests.swift index 8724a0d03..840ea7732 100644 --- a/Tests/BookmarksTests/BookmarkMigrationTests.swift +++ b/Tests/BookmarksTests/BookmarkMigrationTests.swift @@ -98,7 +98,7 @@ class BookmarkMigrationTests: XCTestCase { func commonMigrationTestForDatabase(name: String) throws { try copyDatabase(name: name, formDirectory: resourceURLDir, toDirectory: location) - let legacyFavoritesInOrder = BookmarkFormFactorFavoritesMigration.getFavoritesOrderFromPreV4Model(dbContainerLocation: location, + let legacyFavoritesInOrder = try? BookmarkFormFactorFavoritesMigration().getFavoritesOrderFromPreV4Model(dbContainerLocation: location, dbFileURL: location.appendingPathComponent("\(name).sqlite", conformingTo: .database)) // Now perform migration and test it diff --git a/Tests/BookmarksTests/BookmarkUtilsTests.swift b/Tests/BookmarksTests/BookmarkUtilsTests.swift index 5335cd883..95488ec29 100644 --- a/Tests/BookmarksTests/BookmarkUtilsTests.swift +++ b/Tests/BookmarksTests/BookmarkUtilsTests.swift @@ -48,6 +48,77 @@ final class BookmarkUtilsTests: XCTestCase { try? FileManager.default.removeItem(at: location) } + func testThatFetchingRootFolderPicksTheOneWithMostChildren() { + let context = bookmarksDatabase.makeContext(concurrencyType: .privateQueueConcurrencyType) + context.performAndWait { + BookmarkUtils.prepareFoldersStructure(in: context) + try! context.save() + + guard let root1 = BookmarkUtils.fetchRootFolder(context) else { + XCTFail("root required") + return + } + + let root2 = BookmarkEntity.makeFolder(title: root1.title!, parent: root1, context: context) + root2.uuid = root1.uuid + root2.parent = nil + + let root3 = BookmarkEntity.makeFolder(title: root1.title!, parent: root1, context: context) + root3.uuid = root1.uuid + root3.parent = nil + + _ = BookmarkEntity.makeBookmark(title: "a", url: "a", parent: root2, context: context) + _ = BookmarkEntity.makeBookmark(title: "b", url: "b", parent: root2, context: context) + _ = BookmarkEntity.makeBookmark(title: "c", url: "c", parent: root1, context: context) + + try! context.save() + + let root = BookmarkUtils.fetchRootFolder(context) + XCTAssertEqual(root, root2) + } + } + + func testThatFetchingRootFavoritesFolderPicksTheOneWithMostFavorites() { + let context = bookmarksDatabase.makeContext(concurrencyType: .privateQueueConcurrencyType) + context.performAndWait { + BookmarkUtils.prepareFoldersStructure(in: context) + try! context.save() + + guard let root = BookmarkUtils.fetchRootFolder(context), + let mobileFav1 = BookmarkUtils.fetchFavoritesFolder(withUUID: FavoritesFolderID.mobile.rawValue, in: context), + let unifiedFav1 = BookmarkUtils.fetchFavoritesFolder(withUUID: FavoritesFolderID.unified.rawValue, in: context) else { + XCTFail("root required") + return + } + + let mobileFav2 = BookmarkEntity.makeFolder(title: mobileFav1.title!, parent: root, context: context) + mobileFav2.uuid = mobileFav1.uuid + mobileFav2.parent = nil + + let unifiedFav2 = BookmarkEntity.makeFolder(title: unifiedFav1.title!, parent: root, context: context) + unifiedFav2.uuid = unifiedFav1.uuid + unifiedFav2.parent = nil + + let bA = BookmarkEntity.makeBookmark(title: "a", url: "a", parent: root, context: context) + let bB = BookmarkEntity.makeBookmark(title: "b", url: "b", parent: root, context: context) + + bA.addToFavorites(favoritesRoot: mobileFav2) + bA.addToFavorites(favoritesRoot: unifiedFav1) + bB.addToFavorites(folders: [mobileFav2, unifiedFav1]) + + try! context.save() + + let roots = BookmarkUtils.fetchFavoritesFolders(for: .displayUnified(native: .mobile), in: context) + XCTAssertEqual(Set(roots.map { $0.uuid }), [mobileFav2.uuid, unifiedFav1.uuid]) + + let mobileFav = BookmarkUtils.fetchFavoritesFolder(withUUID: FavoritesFolderID.mobile.rawValue, in: context) + XCTAssertEqual(mobileFav, mobileFav2) + + let unifiedFav = BookmarkUtils.fetchFavoritesFolder(withUUID: FavoritesFolderID.unified.rawValue, in: context) + XCTAssertEqual(unifiedFav, unifiedFav1) + } + } + func testCopyFavoritesWhenDisablingSyncInDisplayNativeMode() async throws { let context = bookmarksDatabase.makeContext(concurrencyType: .privateQueueConcurrencyType)