Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core Data: Optimize storage usage in ProductReviewStore #14134

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Storage/Storage/Protocols/StorageManagerType.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ public extension NSNotification.Name {

/// Defines the methods and properties implemented by any concrete StorageManager implementation.
///
public protocol StorageManagerType {
public protocol StorageManagerType: AnyObject {

/// Returns the `Storage` associated to the main thread.
///
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@

import Foundation
import Networking
import protocol Storage.StorageType
import protocol Storage.StorageManagerType

/// Fetches the `Note`, `ProductReview`, and `Product` in sequence from the Storage and/or API
/// using a `noteID`.
Expand Down Expand Up @@ -43,36 +43,31 @@ final class RetrieveProductReviewFromNoteUseCase {
private let productReviewsRemote: ProductReviewsRemoteProtocol
private let productsRemote: ProductsRemoteProtocol

/// The derived `StorageType` used by the `ProductReviewStore`.
///
/// Storage Layer
/// We should use `weak` because we have to guarantee that we will not do any saving if this
/// `StorageType` is deallocated, which is part of the `ProductReviewStore` lifecycle.
/// `StorageManagerType` is deallocated, which is part of the `ProductReviewStore` lifecycle.
///
private weak var derivedStorage: StorageType?
private weak var storageManager: StorageManagerType?

/// Create an instance of self.
///
/// - Parameters:
/// - derivedStorage: The derived (background) `StorageType` of `ProductReviewStore`.
init(derivedStorage: StorageType,
notificationsRemote: NotificationsRemoteProtocol,
init(notificationsRemote: NotificationsRemoteProtocol,
productReviewsRemote: ProductReviewsRemoteProtocol,
productsRemote: ProductsRemoteProtocol) {
self.derivedStorage = derivedStorage
productsRemote: ProductsRemoteProtocol,
storageManager: StorageManagerType) {
self.notificationsRemote = notificationsRemote
self.productReviewsRemote = productReviewsRemote
self.productsRemote = productsRemote
self.storageManager = storageManager
}

/// Create an instance of self.
///
/// - Parameters:
/// - derivedStorage: The derived (background) `StorageType` of `ProductReviewStore`.
convenience init(network: Network, derivedStorage: StorageType) {
self.init(derivedStorage: derivedStorage,
notificationsRemote: NotificationsRemote(network: network),
convenience init(network: Network, storageManager: StorageManagerType) {
self.init(notificationsRemote: NotificationsRemote(network: network),
productReviewsRemote: ProductReviewsRemote(network: network),
productsRemote: ProductsRemote(network: network))
productsRemote: ProductsRemote(network: network),
storageManager: storageManager)
}

/// Retrieve the `Note`, `ProductReview`, and `Product` based on the given `noteID`.
Expand Down Expand Up @@ -101,7 +96,7 @@ final class RetrieveProductReviewFromNoteUseCase {
private func fetchNote(noteID: Int64,
abort: @escaping AbortBlock,
next: @escaping (Note) -> Void) {
if let noteInStorage = derivedStorage?.loadNotification(noteID: noteID) {
if let noteInStorage = storageManager?.viewStorage.loadNotification(noteID: noteID) {
return next(noteInStorage.toReadOnly())
}

Expand Down Expand Up @@ -129,7 +124,7 @@ final class RetrieveProductReviewFromNoteUseCase {
return abort(ProductReviewFromNoteRetrieveError.reviewNotFound)
}

if let productReviewInStorage = derivedStorage?.loadProductReview(siteID: Int64(siteID), reviewID: Int64(reviewID)) {
if let productReviewInStorage = storageManager?.viewStorage.loadProductReview(siteID: Int64(siteID), reviewID: Int64(reviewID)) {
return next(productReviewInStorage.toReadOnly())
}

Expand All @@ -148,17 +143,14 @@ final class RetrieveProductReviewFromNoteUseCase {
private func saveProductReview(_ review: ProductReview,
abort: @escaping AbortBlock,
next: @escaping () -> Void) {
guard let derivedStorage = derivedStorage else {
guard let storageManager else {
return abort(ProductReviewFromNoteRetrieveError.storageNoLongerAvailable)
}

derivedStorage.perform {
let storageReview = derivedStorage.loadProductReview(siteID: review.siteID, reviewID: review.reviewID)
?? derivedStorage.insertNewObject(ofType: StorageProductReview.self)
storageManager.performAndSave({ storage in
let storageReview = storage.loadProductReview(siteID: review.siteID, reviewID: review.reviewID)
?? storage.insertNewObject(ofType: StorageProductReview.self)
storageReview.update(with: review)

DispatchQueue.main.async(execute: next)
}
}, completion: next, on: .main)
}

/// Fetch the `Product` from storage, or from the API if it is not available in storage.
Expand All @@ -167,7 +159,7 @@ final class RetrieveProductReviewFromNoteUseCase {
productID: Int64,
abort: @escaping AbortBlock,
next: @escaping (Product) -> Void) {
if let productInStorage = derivedStorage?.loadProduct(siteID: siteID, productID: productID) {
if let productInStorage = storageManager?.viewStorage.loadProduct(siteID: siteID, productID: productID) {
return next(productInStorage.toReadOnly())
}

Expand Down
72 changes: 33 additions & 39 deletions Yosemite/Yosemite/Stores/ProductReviewStore.swift
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,7 @@ public final class ProductReviewStore: Store {
private let remote: ProductReviewsRemote

private lazy var productReviewFromNoteUseCase =
RetrieveProductReviewFromNoteUseCase(network: network, derivedStorage: sharedDerivedStorage)

private lazy var sharedDerivedStorage: StorageType = {
return storageManager.writerDerivedStorage
}()
RetrieveProductReviewFromNoteUseCase(network: network, storageManager: storageManager)

public override init(dispatcher: Dispatcher, storageManager: StorageManagerType, network: Network) {
self.remote = ProductReviewsRemote(network: network)
Expand Down Expand Up @@ -65,13 +61,13 @@ private extension ProductReviewStore {

/// Deletes all of the Stored ProductReviews.
///
func resetStoredProductReviews(onCompletion: () -> Void) {
let storage = storageManager.viewStorage
storage.deleteAllObjects(ofType: Storage.ProductReview.self)
storage.saveIfNeeded()
DDLogDebug("Product Reviews deleted")

onCompletion()
func resetStoredProductReviews(onCompletion: @escaping () -> Void) {
storageManager.performAndSave({ storage in
storage.deleteAllObjects(ofType: Storage.ProductReview.self)
}, completion: {
DDLogDebug("Product Reviews deleted")
onCompletion()
}, on: .main)
}

/// Synchronizes the product reviews associated with a given Site ID (if any!).
Expand Down Expand Up @@ -108,9 +104,12 @@ private extension ProductReviewStore {
switch result {
case .failure(let error):
if case NetworkError.notFound = error {
self.deleteStoredProductReview(siteID: siteID, reviewID: reviewID)
self.deleteStoredProductReview(siteID: siteID, reviewID: reviewID) {
onCompletion(nil, error)
}
} else {
onCompletion(nil, error)
}
onCompletion(nil, error)
case .success(let productReview):
self.upsertStoredProductReviewsInBackground(readOnlyProductReviews: [productReview], siteID: siteID) {
onCompletion(productReview, nil)
Expand Down Expand Up @@ -158,14 +157,13 @@ private extension ProductReviewStore {

/// Deletes any Storage.ProductReview with the specified `siteID` and `reviewID`
///
func deleteStoredProductReview(siteID: Int64, reviewID: Int64) {
let storage = storageManager.viewStorage
guard let productReview = storage.loadProductReview(siteID: siteID, reviewID: reviewID) else {
return
}

storage.deleteObject(productReview)
storage.saveIfNeeded()
func deleteStoredProductReview(siteID: Int64, reviewID: Int64, onCompletion: @escaping () -> Void) {
storageManager.performAndSave({ storage in
guard let productReview = storage.loadProductReview(siteID: siteID, reviewID: reviewID) else {
return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated to this PR but while reading the code, I wonder if it's worth adding a log here?

}
storage.deleteObject(productReview)
}, completion: onCompletion, on: .main)
}

/// Updates (OR Inserts) the specified ReadOnly ProductReview Entities *in a background thread*. onCompletion will be called
Expand All @@ -175,32 +173,28 @@ private extension ProductReviewStore {
siteID: Int64,
shouldDeleteAllStoredProductReviews: Bool = false,
onCompletion: @escaping () -> Void) {
let derivedStorage = sharedDerivedStorage
derivedStorage.perform {
storageManager.performAndSave({ [weak self] storage in
if shouldDeleteAllStoredProductReviews {
derivedStorage.deleteProductReviews(siteID: siteID)
storage.deleteProductReviews(siteID: siteID)
}
self.upsertStoredProductReviews(readOnlyProductReviews: readOnlyProductReviews, in: derivedStorage, siteID: siteID)
}

storageManager.saveDerivedType(derivedStorage: derivedStorage) {
DispatchQueue.main.async(execute: onCompletion)
}
self?.upsertStoredProductReviews(readOnlyProductReviews: readOnlyProductReviews, in: storage, siteID: siteID)
}, completion: onCompletion, on: .main)
}

func moderateReview(siteID: Int64, reviewID: Int64, status: ProductReviewStatus, onCompletion: @escaping (ProductReviewStatus?, Error?) -> Void) {
let storage = storageManager.viewStorage
remote.updateProductReviewStatus(for: siteID, reviewID: reviewID, statusKey: status.rawValue) { (productReview, error) in
guard let productReview = productReview else {
remote.updateProductReviewStatus(for: siteID, reviewID: reviewID, statusKey: status.rawValue) { [weak self] (productReview, error) in
guard let self, let productReview else {
onCompletion(nil, error)
return
}

if let existingStorageProductReview = storage.loadProductReview(siteID: siteID, reviewID: reviewID) {
existingStorageProductReview.update(with: productReview)
}

onCompletion(productReview.status, nil)
storageManager.performAndSave({ storage in
if let existingStorageProductReview = storage.loadProductReview(siteID: siteID, reviewID: reviewID) {
existingStorageProductReview.update(with: productReview)
}
}, completion: {
onCompletion(productReview.status, nil)
}, on: .main)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -285,10 +285,10 @@ private extension RetrieveProductReviewFromNoteUseCaseTests {
/// Create a UseCase using the mocks
///
func makeUseCase() -> RetrieveProductReviewFromNoteUseCase {
RetrieveProductReviewFromNoteUseCase(derivedStorage: viewStorage,
notificationsRemote: notificationsRemote,
RetrieveProductReviewFromNoteUseCase(notificationsRemote: notificationsRemote,
productReviewsRemote: productReviewsRemote,
productsRemote: productsRemote)
productsRemote: productsRemote,
storageManager: storageManager)
}

/// Retrieve the Parcel using the given UseCase
Expand Down
Loading