From 7345bf7064b43f4cb57d40edb2caaba6cf9edfc6 Mon Sep 17 00:00:00 2001 From: David Skuza Date: Thu, 23 Mar 2023 16:11:50 -0500 Subject: [PATCH 1/2] fix(cache): correctly manage fetching and deleting of images --- .../Sources/PocketKit/ImageManager.swift | 98 +++++++++++-------- PocketKit/Sources/PocketKit/Services.swift | 2 +- PocketKit/Sources/Sync/ImagesController.swift | 28 ------ PocketKit/Sources/Sync/PocketSource.swift | 11 +-- PocketKit/Sources/Sync/Requests.swift | 4 +- PocketKit/Sources/Sync/Source.swift | 4 +- PocketKit/Sources/Sync/Space.swift | 2 +- 7 files changed, 66 insertions(+), 83 deletions(-) diff --git a/PocketKit/Sources/PocketKit/ImageManager.swift b/PocketKit/Sources/PocketKit/ImageManager.swift index 09da6cdd7..792a7ff6b 100644 --- a/PocketKit/Sources/PocketKit/ImageManager.swift +++ b/PocketKit/Sources/PocketKit/ImageManager.swift @@ -11,6 +11,8 @@ protocol ImageCacheProtocol { callbackQueue: CallbackQueue, completionHandler: (() -> Void)? ) + + func isCached(forKey key: String, processorIdentifier identifier: String) -> Bool } extension ImageCache: ImageCacheProtocol { } @@ -53,77 +55,91 @@ class ImageManager { imagesController.delegate = self try? imagesController.performFetch() - imagesController.images?.forEach { download(image: $0) } + handle(images: imagesController.images) } } private extension ImageManager { - func download(image: Image, _ completion: ((Bool) -> Void)? = nil) { - guard let source = image.source, let cachedSource = imageCacheURL(for: source) else { + func download(url: URL, _ completion: ((Bool) -> Void)? = nil) { + // 1. Check if we have a valid image cache url + // 2. Check if the image is already cached + // If the image has a valid url and is already cached, skip; else, retrieve + guard let cachedURL = imageCacheURL(for: url), + imageRetriever.imageCache.isCached( + forKey: cachedURL.cacheKey, + processorIdentifier: DefaultImageProcessor.default.identifier + ) == false else { return } imageRetriever.retrieveImage( - with: cachedSource, + with: cachedURL, options: nil, progressBlock: nil, downloadTaskUpdated: nil ) { result in - switch result { - case .success: - completion?(true) - default: - completion?(false) - } + switch result { + case .success: + completion?(true) + default: + completion?(false) } + } } - func delete(image: Image) { - guard let source = image.source, let cachedSource = imageCacheURL(for: source) else { + func delete(url: URL) { + // 1. Check if we have a valid image cache url + // 2. Check if the image is already cached + // If the image has a valid url and is not already cached, skip; else, delete + guard let cachedURL = imageCacheURL(for: url), + imageRetriever.imageCache.isCached( + forKey: cachedURL.cacheKey, + processorIdentifier: DefaultImageProcessor.default.identifier + ) == true else { return } imageRetriever.imageCache.removeImage( - forKey: cachedSource.cacheKey, - processorIdentifier: "", + forKey: cachedURL.cacheKey, + processorIdentifier: DefaultImageProcessor.default.identifier, fromMemory: true, fromDisk: true, callbackQueue: .untouch, completionHandler: nil ) } -} -extension ImageManager: ImagesControllerDelegate { - func controller( - _ controller: ImagesController, - didChange image: Image, - at indexPath: IndexPath?, - for type: NSFetchedResultsChangeType, - newIndexPath: IndexPath? - ) { - switch type { - case .insert, .update: - download(image: image) { [weak self] success in - if success { - self?.source.download(images: [image]) - } - } - case .delete: - delete(image: image) - case .move: - return - @unknown default: + func handle(images: [Image]?) { + guard let images = images, !images.isEmpty else { return } + + // Images are removed via `removeFromImages` when Items are updated + // This nullifies the item relationship from Image -> Item + // Therefore, we want to retrieve orphaned Images so we can delete them + let orphans = images.filter { $0.item == nil } + + let allURLs = Set(images.compactMap { $0.source }) + let orphanURLs = Set(orphans.compactMap { $0.source }) + + // All URLs that are not orphans + let toDownload = allURLs.subtracting(orphanURLs) + // Skip deleting any URLs that are also orphans, as to not redownload + let skipDelete = toDownload.intersection(orphanURLs) + // Delete all orphan URLs that are not to be skipped + let toDelete = orphanURLs.subtracting(skipDelete) + + toDelete.forEach { delete(url: $0) } + toDownload.forEach { download(url: $0) } + + source.delete(images: orphans) } +} +extension ImageManager: ImagesControllerDelegate { func controllerDidChangeContent(_ controller: ImagesController) { - guard let images = controller.images, !images.isEmpty else { - return - } - - images.forEach { download(image: $0) } - source.download(images: images) + // Called once all context changes are saved (inserts, deletes, etc) + // so we can bulk handle the latest Images + handle(images: controller.images) } } diff --git a/PocketKit/Sources/PocketKit/Services.swift b/PocketKit/Sources/PocketKit/Services.swift index fa47a5463..dc40648b9 100644 --- a/PocketKit/Sources/PocketKit/Services.swift +++ b/PocketKit/Sources/PocketKit/Services.swift @@ -85,7 +85,7 @@ struct Services { ) imageManager = ImageManager( - imagesController: source.makeUndownloadedImagesController(), + imagesController: source.makeImagesController(), imageRetriever: KingfisherManager.shared, source: source ) diff --git a/PocketKit/Sources/Sync/ImagesController.swift b/PocketKit/Sources/Sync/ImagesController.swift index ada03ae26..6435fd0d0 100644 --- a/PocketKit/Sources/Sync/ImagesController.swift +++ b/PocketKit/Sources/Sync/ImagesController.swift @@ -2,14 +2,6 @@ import Foundation import CoreData public protocol ImagesControllerDelegate: AnyObject { - func controller( - _ controller: ImagesController, - didChange image: Image, - at indexPath: IndexPath?, - for type: NSFetchedResultsChangeType, - newIndexPath: IndexPath? - ) - func controllerDidChangeContent(_ controller: ImagesController) } @@ -44,26 +36,6 @@ class FetchedImagesController: NSObject, ImagesController { } extension FetchedImagesController: NSFetchedResultsControllerDelegate { - func controller( - _ controller: NSFetchedResultsController, - didChange anObject: Any, - at indexPath: IndexPath?, - for type: NSFetchedResultsChangeType, - newIndexPath: IndexPath? - ) { - guard let image = anObject as? Image else { - return - } - - delegate?.controller( - self, - didChange: image, - at: indexPath, - for: type, - newIndexPath: newIndexPath - ) - } - func controllerDidChangeContent(_ controller: NSFetchedResultsController) { delegate?.controllerDidChangeContent(self) } diff --git a/PocketKit/Sources/Sync/PocketSource.swift b/PocketKit/Sources/Sync/PocketSource.swift index 9235c70df..e15c4d398 100644 --- a/PocketKit/Sources/Sync/PocketSource.swift +++ b/PocketKit/Sources/Sync/PocketSource.swift @@ -150,8 +150,8 @@ public class PocketSource: Source { PocketSearchService(apollo: apollo) } - public func makeUndownloadedImagesController() -> ImagesController { - FetchedImagesController(resultsController: space.makeUndownloadedImagesController()) + public func makeImagesController() -> ImagesController { + FetchedImagesController(resultsController: space.makeImagesController()) } public func makeRecentSavesController() -> NSFetchedResultsController { @@ -716,11 +716,8 @@ extension PocketSource { // MARK: - Image extension PocketSource { - public func download(images: [Image]) { - images.forEach { - $0.isDownloaded = true - } - + public func delete(images: [Image]) { + space.delete(images) try? space.save() } } diff --git a/PocketKit/Sources/Sync/Requests.swift b/PocketKit/Sources/Sync/Requests.swift index 811f475a8..d567e5e27 100644 --- a/PocketKit/Sources/Sync/Requests.swift +++ b/PocketKit/Sources/Sync/Requests.swift @@ -203,9 +203,7 @@ public enum Requests { } public static func fetchUndownloadedImages() -> NSFetchRequest { - let request = Image.fetchRequest() - request.predicate = NSPredicate(format: "isDownloaded = NO") - return request + return Image.fetchRequest() } public static func fetchSavedItem(for item: Item) -> NSFetchRequest { diff --git a/PocketKit/Sources/Sync/Source.swift b/PocketKit/Sources/Sync/Source.swift index b6083bf8c..68229db27 100644 --- a/PocketKit/Sources/Sync/Source.swift +++ b/PocketKit/Sources/Sync/Source.swift @@ -32,7 +32,7 @@ public protocol Source { func makeSearchService() -> SearchService - func makeUndownloadedImagesController() -> ImagesController + func makeImagesController() -> ImagesController func refreshSaves(completion: (() -> Void)?) @@ -86,7 +86,7 @@ public protocol Source { func remove(recommendation: Recommendation) - func download(images: [Image]) + func delete(images: [Image]) func fetchDetails(for savedItem: SavedItem) async throws diff --git a/PocketKit/Sources/Sync/Space.swift b/PocketKit/Sources/Sync/Space.swift index d8454644c..1c6206496 100644 --- a/PocketKit/Sources/Sync/Space.swift +++ b/PocketKit/Sources/Sync/Space.swift @@ -260,7 +260,7 @@ public class Space { ) } - func makeUndownloadedImagesController() -> NSFetchedResultsController { + func makeImagesController() -> NSFetchedResultsController { let request = Requests.fetchUndownloadedImages() request.sortDescriptors = [NSSortDescriptor(key: "source.absoluteString", ascending: true)] return NSFetchedResultsController( From e9d515a3837bca5c04ef337309018706187bbcb0 Mon Sep 17 00:00:00 2001 From: David Skuza Date: Thu, 23 Mar 2023 16:33:53 -0500 Subject: [PATCH 2/2] refactor(tests): update ImageManager tests to reflect latest changes --- .../PocketKitTests/ImageManagerTests.swift | 147 ++++++++---------- .../Support/MockImageDownloader.swift | 35 +++++ .../PocketKitTests/Support/MockSource.swift | 36 ++--- .../Support/Space+factories.swift | 13 +- .../Tests/SyncTests/PocketSourceTests.swift | 9 -- 5 files changed, 131 insertions(+), 109 deletions(-) diff --git a/PocketKit/Tests/PocketKitTests/ImageManagerTests.swift b/PocketKit/Tests/PocketKitTests/ImageManagerTests.swift index 68aca5811..5c7f2b751 100644 --- a/PocketKit/Tests/PocketKitTests/ImageManagerTests.swift +++ b/PocketKit/Tests/PocketKitTests/ImageManagerTests.swift @@ -22,7 +22,7 @@ class ImageManagerTests: XCTestCase { imagesController.stubPerformFetch { } imageCache.stubRemoveImage { _, _, _, _, _, _ in } imageRetriever.stubRetrieveImage { _, _, _, _, _ in return nil } - source.stubDownloadImages { _ in } + source.stubDeleteImages { _ in } } override func tearDownWithError() throws { @@ -41,103 +41,92 @@ class ImageManagerTests: XCTestCase { ) } - func test_whenImagesInsertedOrUpdated_downloadsEachImage() { - imageRetriever.stubRetrieveImage { _, _, _, _, completion in - let result = RetrieveImageResult( - image: UIImage(), - cacheType: .memory, - source: .network(URL(string: "https://getpocket.com/example-image.png")!), - originalSource: .network(URL(string: "https://getpocket.com/example-image.png")!), - data: { - return nil - } + func test_onStart_withNoOrphans_andNoCachedImages_downloadsImages() { + imagesController.images = [ + try! space.createImage( + source: URL(string: "https://getpocket.com"), + item: try! space.createItem() ) - completion?(.success(result)) - return nil + ] + + imageCache.stubIsCached { _, _ in + return false } - let prefetcher = subject() - prefetcher.start() + let subject = subject() - let images: [Image] = [ - space.buildImage(source: URL(string: "https://example.com/image-1.png")), - space.buildImage(source: URL(string: "https://example.com/image-2.png")), - space.buildImage(source: URL(string: "https://example.com/image-3.png")), - space.buildImage(source: URL(string: "https://example.com/image-4.png")), - ] - imagesController.images = Array(images[0...1]) + subject.start() - imagesController.delegate?.controllerDidChangeContent(imagesController) + let resource = imageRetriever.retrieveImageCall(at: 0)?.resource + let expectedURL = imageCacheURL(for: imagesController.images!.first!.source) - XCTAssertEqual(imageRetriever.retrieveImageCall(at: 0)?.resource as? URL, imageCacheURL(for: images[0].source)) - XCTAssertEqual(source.downloadImagesCall(at: 0)?.images.first, images[0]) - XCTAssertEqual(imageRetriever.retrieveImageCall(at: 1)?.resource as? URL, imageCacheURL(for: images[1].source)) - XCTAssertEqual(source.downloadImagesCall(at: 0)?.images, imagesController.images) + // Force unwrapping also tests for nil; two-in-one + XCTAssertEqual(resource!.downloadURL, expectedURL) + } - imagesController.delegate?.controller( - imagesController, - didChange: images[2], - at: nil, - for: .insert, - newIndexPath: nil - ) + func test_onStart_withNoOrphans_andCachedImages_doesNothing() { + imagesController.images = [ + try! space.createImage( + source: URL(string: "https://getpocket.com"), + item: try! space.createItem() + ) + ] - XCTAssertEqual(imageRetriever.retrieveImageCall(at: 2)?.resource as? URL, imageCacheURL(for: images[2].source)) - XCTAssertEqual(source.downloadImagesCall(at: 1)?.images, [images[2]]) + imageCache.stubIsCached { _, _ in + return true + } - imagesController.delegate?.controller( - imagesController, - didChange: images[3], - at: nil, - for: .update, - newIndexPath: nil - ) + let subject = subject() - XCTAssertEqual(imageRetriever.retrieveImageCall(at: 3)?.resource as? URL, imageCacheURL(for: images[3].source)) - XCTAssertEqual(source.downloadImagesCall(at: 2)?.images, [images[3]]) - } + subject.start() - func test_whenImagesAreMoved_doesNothing() { - let prefetcher = subject() - prefetcher.start() + XCTAssertNil(imageRetriever.retrieveImageCall(at: 0)) + } - let images: [Image] = [ - space.buildImage(source: URL(string: "https://example.com/image-1.png")), + func test_onStart_withOrphans_andNoCachedImages_deletesImagesFromSource() { + imagesController.images = [ + try! space.createImage( + source: URL(string: "https://getpocket.com/"), + item: try! space.createItem() + ), + try! space.createImage( + source: URL(string: "https://getpocket.com/orphan") + ) ] - imagesController.images = images - - imagesController.delegate?.controller( - imagesController, - didChange: images[0], - at: nil, - for: .move, - newIndexPath: nil - ) - XCTAssertNil(imageRetriever.retrieveImageCall(at: 0)) - XCTAssertNil(source.downloadImagesCall(at: 0)) - } + imageCache.stubIsCached { _, _ in + return false + } - func test_whenImagesAreDeleted_removesImageFromCache() { - let prefetcher = subject() - prefetcher.start() + let subject = subject() - let images: [Image] = [ - space.buildImage(source: URL(string: "https://example.com/image-1.png")), - ] - imagesController.images = images - - imagesController.delegate?.controller( - imagesController, - didChange: images[0], - at: nil, - for: .delete, - newIndexPath: nil + subject.start() + + XCTAssertEqual( + source.deleteImagesCall(at: 0)!.images[0], + imagesController.images![1] ) + } + + func test_onStart_withOrphans_andCachedImages_removesOrphansFromCache() { + imagesController.images = [ + try! space.createImage( + source: URL(string: "https://getpocket.com/orphan") + ) + ] + + imageCache.stubIsCached { _, _ in + return true + } + + let subject = subject() + + subject.start() + let expectedKey = imageCacheURL(for: imagesController.images![0].source!)!.absoluteString XCTAssertEqual( - imageCache.removeImageCall(at: 0)?.key, - imageCacheURL(for: images[0].source!)!.cacheKey + imageCache.removeImageCall(at: 0)!.key, + expectedKey ) } } diff --git a/PocketKit/Tests/PocketKitTests/Support/MockImageDownloader.swift b/PocketKit/Tests/PocketKitTests/Support/MockImageDownloader.swift index 6049b139b..c73664174 100644 --- a/PocketKit/Tests/PocketKitTests/Support/MockImageDownloader.swift +++ b/PocketKit/Tests/PocketKitTests/Support/MockImageDownloader.swift @@ -67,6 +67,41 @@ extension MockImageCache { } } +extension MockImageCache { + private static let isCached = "isCached" + + typealias IsCachedImpl = (String, String) -> Bool + + struct IsCachedCall { + let key: String + let processIdentifier: String + } + + func stubIsCached(_ impl: @escaping IsCachedImpl) { + implementations[Self.isCached] = impl + } + + func isCachedCall(at index: Int) -> IsCachedCall? { + guard let calls = calls[Self.isCached], index < calls.count else { + return nil + } + + return calls[index] as? IsCachedCall + } + + func isCached(forKey key: String, processorIdentifier identifier: String) -> Bool { + guard let impl = implementations[Self.isCached] as? IsCachedImpl else { + fatalError("\(Self.self).\(#function) is not stubbed") + } + + calls[Self.isCached] = (calls[Self.isCached] ?? []) + [ + IsCachedCall(key: key, processIdentifier: identifier) + ] + + return impl(key, identifier) + } +} + class MockImageRetriever: ImageRetriever { private var implementations: [String: Any] = [:] private var calls: [String: [Any]] = [:] diff --git a/PocketKit/Tests/PocketKitTests/Support/MockSource.swift b/PocketKit/Tests/PocketKitTests/Support/MockSource.swift index 525548600..fc6bae413 100644 --- a/PocketKit/Tests/PocketKitTests/Support/MockSource.swift +++ b/PocketKit/Tests/PocketKitTests/Support/MockSource.swift @@ -306,15 +306,15 @@ extension MockSource { // MARK: - Make images controller extension MockSource { - static let makeUndownloadedImagesController = "makeUndownloadedImagesController" - typealias MakeUndownloadedImagesControllerImpl = () -> ImagesController + static let makeImagesController = "makeImagesController" + typealias MakeImagesControllerImpl = () -> ImagesController - func stubMakeUndownloadedImagesController(impl: @escaping MakeUndownloadedImagesControllerImpl) { - implementations[Self.makeUndownloadedImagesController] = impl + func stubMakeImagesController(impl: @escaping MakeImagesControllerImpl) { + implementations[Self.makeImagesController] = impl } - func makeUndownloadedImagesController() -> ImagesController { - guard let impl = implementations[Self.makeUndownloadedImagesController] as? MakeUndownloadedImagesControllerImpl else { + func makeImagesController() -> ImagesController { + guard let impl = implementations[Self.makeImagesController] as? MakeImagesControllerImpl else { fatalError("\(Self.self).\(#function) has not been stubbed") } @@ -1012,33 +1012,33 @@ extension MockSource { } extension MockSource { - private static let downloadImages = "downloadImages" - typealias DownloadImagesImpl = ([Image]) -> Void - struct DownloadImagesCall { + private static let deleteImages = "deleteImages" + typealias DeleteImagesImpl = ([Image]) -> Void + struct DeleteImagesCall { let images: [Image] } - func stubDownloadImages(_ impl: @escaping DownloadImagesImpl) { - implementations[Self.downloadImages] = impl + func stubDeleteImages(_ impl: @escaping DeleteImagesImpl) { + implementations[Self.deleteImages] = impl } - func downloadImagesCall(at index: Int) -> DownloadImagesCall? { - guard let calls = calls[Self.downloadImages], + func deleteImagesCall(at index: Int) -> DeleteImagesCall? { + guard let calls = calls[Self.deleteImages], index < calls.count, - let call = calls[index] as? DownloadImagesCall else { + let call = calls[index] as? DeleteImagesCall else { return nil } return call } - func download(images: [Image]) { - guard let impl = implementations[Self.downloadImages] as? DownloadImagesImpl else { + func delete(images: [Image]) { + guard let impl = implementations[Self.deleteImages] as? DeleteImagesImpl else { fatalError("\(Self.self).\(#function) has not been stubbed") } - calls[Self.downloadImages] = (calls[Self.downloadImages] ?? []) + [ - DownloadImagesCall(images: images) + calls[Self.deleteImages] = (calls[Self.deleteImages] ?? []) + [ + DeleteImagesCall(images: images) ] impl(images) diff --git a/PocketKit/Tests/PocketKitTests/Support/Space+factories.swift b/PocketKit/Tests/PocketKitTests/Support/Space+factories.swift index d7bb36a89..21ae8dd91 100644 --- a/PocketKit/Tests/PocketKitTests/Support/Space+factories.swift +++ b/PocketKit/Tests/PocketKitTests/Support/Space+factories.swift @@ -296,12 +296,14 @@ extension Space { @discardableResult func buildImage( source: URL?, - isDownloaded: Bool = false + isDownloaded: Bool = false, + item: Item? = nil ) -> Image { return backgroundContext.performAndWait { let image: Image = Image(context: backgroundContext) image.source = source image.isDownloaded = isDownloaded + image.item = item return image } @@ -310,10 +312,15 @@ extension Space { @discardableResult func createImage( source: URL?, - isDownloaded: Bool = false + isDownloaded: Bool = false, + item: Item? = nil ) throws -> Image { return try backgroundContext.performAndWait { - let image = buildImage(source: source, isDownloaded: isDownloaded) + let image = buildImage( + source: source, + isDownloaded: isDownloaded, + item: item + ) try backgroundContext.save() return image diff --git a/PocketKit/Tests/SyncTests/PocketSourceTests.swift b/PocketKit/Tests/SyncTests/PocketSourceTests.swift index 9abf37656..e171858b1 100644 --- a/PocketKit/Tests/SyncTests/PocketSourceTests.swift +++ b/PocketKit/Tests/SyncTests/PocketSourceTests.swift @@ -394,15 +394,6 @@ class PocketSourceTests: XCTestCase { XCTAssertEqual(fetched, [recommendation2]) } - func test_downloadImage_updatesIsDownloadedProperty() throws { - let image: Image = Image(context: space.backgroundContext) - - let source = subject() - source.download(images: [image]) - - XCTAssertTrue(image.isDownloaded) - } - @MainActor func test_fetchOfflineContent_fetchesOfflineContent() async throws { apollo.stubFetch(