Tweaks to DB API, improved abstractions for better testability. (#913)

Required:

Task/Issue URL: https://app.asana.com/0/856498667320406/1207875839908777/f
iOS PR: duckduckgo/iOS#3143
macOS PR: duckduckgo/macos-browser#3033
What kind of version bump will this require?: Major

Description:

Tweak code to improve error handling and events propagation in Clients. Add protocols for improved testability.

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<String>, 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? {

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<MigrationErrors>? = 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,16 +64,22 @@ 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
             }
 
             let favs = BookmarkUtils.fetchLegacyFavoritesFolder(context)
             let orderedFavorites = favs?.favorites?.array as? [BookmarkEntity] ?? []
             oldFavoritesOrder = orderedFavorites.compactMap { $0.uuid }
         }
+
+        if let loadError {
+            throw loadError
+        }
+
         return oldFavoritesOrder
     }
 

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

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)

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

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)