Core Data Stack updates

Storage/Storage/CoreData/CoreDataManager.swift

@@ -15,6 +15,9 @@ public final class CoreDataManager: StorageManagerType {
 
     private let modelsInventory: ManagedObjectModelsInventory
 
+    /// A serial queue used to ensure there is only one writing operation at a time.
+    private let writerQueue: OperationQueue
+
     /// Module-private designated Initializer.
     ///
     /// - Parameter name: Identifier to be used for: [database, data model, container].
@@ -30,6 +33,9 @@ public final class CoreDataManager: StorageManagerType {
          modelsInventory: ManagedObjectModelsInventory?) {
         self.name = name
         self.crashLogger = crashLogger
+        self.writerQueue = OperationQueue()
+        self.writerQueue.name = "com.automattic.woocommerce.CoreDataManager.writer"
+        self.writerQueue.maxConcurrentOperationCount = 1
 
         do {
             if let modelsInventory = modelsInventory {
@@ -59,16 +65,20 @@ public final class CoreDataManager: StorageManagerType {
     /// Returns the Storage associated with the View Thread.
     ///
     public var viewStorage: StorageType {
-        return persistentContainer.viewContext
+        let context = persistentContainer.viewContext
+        /// This simplifies the process of merging updates from persistent container to view context.
+        /// When disable auto merge, we need to handle merging manually using `NSManagedObjectContextDidSave` notifications.
+        context.automaticallyMergesChangesFromParent = true
+        context.mergePolicy = NSMergeByPropertyObjectTrumpMergePolicy
+        return context
     }
 
     /// Returns a shared derived storage instance dedicated for write operations.
     ///
     public lazy var writerDerivedStorage: StorageType = {
-        let childManagedObjectContext = NSManagedObjectContext(concurrencyType: .privateQueueConcurrencyType)
-        childManagedObjectContext.parent = persistentContainer.viewContext
-        childManagedObjectContext.mergePolicy = NSMergeByPropertyObjectTrumpMergePolicy
-        return childManagedObjectContext
+        let backgroundContext = persistentContainer.newBackgroundContext()
+        backgroundContext.mergePolicy = NSMergeByPropertyObjectTrumpMergePolicy
+        return backgroundContext
     }()
 
     /// Persistent Container: Holds the full CoreData Stack
@@ -130,25 +140,36 @@ public final class CoreDataManager: StorageManagerType {
         return container
     }()
 
-    /// Performs the received closure in Background. Note that you should use the received Storage instance (BG friendly!).
-    ///
-    public func performBackgroundTask(_ closure: @escaping (StorageType) -> Void) {
-        persistentContainer.performBackgroundTask { context in
-            closure(context as StorageType)
-        }
-    }
-
     /// Saves the derived storage. Note: the closure may be called on a different thread
     ///
     public func saveDerivedType(derivedStorage: StorageType, _ closure: @escaping () -> Void) {
         derivedStorage.perform {
             derivedStorage.saveIfNeeded()
+            closure()
+        }
+    }
 
-            self.viewStorage.perform {
-                self.viewStorage.saveIfNeeded()
-                closure()
+    /// Execute the given block with a background context and save the changes.
+    ///
+    /// This function _does not block_ its running thread. The block is executed in background and its return value
+    /// is passed onto the `completion` block which is executed on the given `queue`.
+    ///
+    /// - Parameters:
+    ///   - closure: A closure which uses the given `NSManagedObjectContext` to make Core Data model changes.
+    ///   - completion: A closure which is called with the return value of the `block`, after the changed made
+    ///         by the `block` is saved.
+    ///   - queue: A queue on which to execute the completion block.
+    public func performAndSave(_ closure: @escaping (StorageType) -> Void, completion: (() -> Void)?, on queue: DispatchQueue) {
+        let derivedStorage = writerDerivedStorage
+        self.writerQueue.addOperation(AsyncBlockOperation { done in
+            derivedStorage.perform {
+                closure(derivedStorage)
+
+                derivedStorage.saveIfNeeded()
+                queue.async { completion?() }
+                done()
             }
-        }
+        })
     }
 
     /// This method effectively destroys all of the stored data, and generates a blank Persistent Store from scratch.
@@ -323,3 +344,66 @@ extension CoreDataManagerError: CustomStringConvertible {
         }
     }
 }
+
+/// Helper types to support writing operations to be handled one by one.
+/// This implementation follows WP/JP's work at
+/// WordPress/Classes/Utility/ContextManager.swift#L131
+///
+extension CoreDataManager {
+    /// Helper type to support handling async operations by keeping track of their states
+    class AsyncOperation: Operation {
+        enum State: String {
+            case isReady, isExecuting, isFinished
+        }
+
+        override var isAsynchronous: Bool {
+            return true
+        }
+
+        var state = State.isReady {
+            willSet {
+                willChangeValue(forKey: state.rawValue)
+                willChangeValue(forKey: newValue.rawValue)
+            }
+            didSet {
+                didChangeValue(forKey: oldValue.rawValue)
+                didChangeValue(forKey: state.rawValue)
+            }
+        }
+
+        override var isExecuting: Bool {
+            return state == .isExecuting
+        }
+
+        override var isFinished: Bool {
+            return state == .isFinished
+        }
+
+        override func start() {
+            if isCancelled {
+                state = .isFinished
+                return
+            }
+
+            state = .isExecuting
+            main()
+        }
+    }
+
+    /// Helper type to handle async operations given a closure of code to be executed.
+    final class AsyncBlockOperation: AsyncOperation {
+
+        private let block: (@escaping () -> Void) -> Void
+
+        init(block: @escaping (@escaping () -> Void) -> Void) {
+            self.block = block
+        }
+
+        override func main() {
+            self.block { [weak self] in
+                self?.state = .isFinished
+            }
+        }
+
+    }
+}

Storage/Storage/Extensions/NSManagedObjectContext+Storage.swift

@@ -60,6 +60,7 @@ extension NSManagedObjectContext: StorageType {
     /// Deletes the specified Object Instance
     ///
     public func deleteObject<T: Object>(_ object: T) {
+        Self.ensureCorrectContextUsageIfNeeded(for: T.entityName)
         guard let object = object as? NSManagedObject else {
             logErrorAndExit("Invalid Object Kind")
         }
@@ -70,6 +71,7 @@ extension NSManagedObjectContext: StorageType {
     /// Deletes all of the NSMO instances associated to the specified kind
     ///
     public func deleteAllObjects<T: Object>(ofType type: T.Type) {
+        Self.ensureCorrectContextUsageIfNeeded(for: T.entityName)
         let request = fetchRequest(forType: type)
         request.includesPropertyValues = false
         request.includesSubentities = false
@@ -104,6 +106,7 @@ extension NSManagedObjectContext: StorageType {
     /// Inserts a new Entity. For performance reasons, this helper *DOES NOT* persists the context.
     ///
     public func insertNewObject<T: Object>(ofType type: T.Type) -> T {
+        Self.ensureCorrectContextUsageIfNeeded(for: T.entityName)
         return NSEntityDescription.insertNewObject(forEntityName: T.entityName, into: self) as! T
     }
 
@@ -134,6 +137,9 @@ extension NSManagedObjectContext: StorageType {
             return
         }
 
+        /// cannot infer the entity name here, so leaving it empty
+        Self.ensureCorrectContextUsageIfNeeded(for: "")
+
         do {
             try save()
         } catch {
@@ -175,4 +181,24 @@ extension NSManagedObjectContext: StorageType {
     private func fetchRequest<T: Object>(forType type: T.Type) -> NSFetchRequest<NSFetchRequestResult> {
         return NSFetchRequest<NSFetchRequestResult>(entityName: type.entityName)
     }
+
+    /// Helper to trigger failure in debug mode or print warning when write operations are called from the view context (main thread).
+    ///
+    private static func ensureCorrectContextUsageIfNeeded(for entityName: String) {
+        #if DEBUG
+        guard Thread.isMainThread else {
+            return
+        }
+        let message: String = {
+            if entityName.isEmpty {
+                "⚠️ Saving data should only be done on a background context"
+            } else {
+                "⚠️ Write operations for \(entityName) should only be done on a background context"
+            }
+        }()
+
+        let enforceWriteInBackground = ProcessInfo.processInfo.arguments.contains("-enforce-core-data-write-in-background")
+        enforceWriteInBackground ? assertionFailure(message) : DDLogWarn(message)
+        #endif
+    }
 }

Storage/Storage/Model/Product+CoreDataProperties.swift

@@ -15,8 +15,8 @@ extension Product {
     @NSManaged public var name: String
     @NSManaged public var slug: String
     @NSManaged public var permalink: String
-    @NSManaged public var date: Date
-    @NSManaged public var dateCreated: Date
+    @NSManaged public var date: Date?
+    @NSManaged public var dateCreated: Date?
     @NSManaged public var dateModified: Date?
     @NSManaged public var dateOnSaleStart: Date?
     @NSManaged public var dateOnSaleEnd: Date?

Storage/Storage/Protocols/StorageManagerType.swift

@@ -18,6 +18,7 @@ public protocol StorageManagerType {
 
     /// Returns a shared derived storage instance dedicated for write operations.
     ///
+    @available(*, deprecated, message: "Use `performAndSave` to handle write operations instead.")
     var writerDerivedStorage: StorageType { get }
 
     /// Performs a task in Background: a special `Storage` instance will be provided (which is expected to be used within the closure!).
@@ -30,8 +31,21 @@ public protocol StorageManagerType {
     ///   - derivedStorageType: a derived StorageType constructed with `newDerivedStorage`
     ///   - closure: Callback to be executed on completion
     ///
+    @available(*, deprecated, message: "Use `performAndSave` to handle write operations instead.")
     func saveDerivedType(derivedStorage: StorageType, _ closure: @escaping () -> Void)
 
+    /// Execute the given block with a background context and save the changes.
+    ///
+    /// This function _does not block_ its running thread. The block is executed in background and its return value
+    /// is passed onto the `completion` block which is executed on the given `queue`.
+    ///
+    /// - Parameters:
+    ///   - closure: A closure which uses the given `NSManagedObjectContext` to make Core Data model changes.
+    ///   - completion: A closure which is called with the return value of the `block`, after the changed made
+    ///         by the `block` is saved.
+    ///   - queue: A queue on which to execute the completion block.
+    func performAndSave(_ closure: @escaping (StorageType) -> Void, completion: (() -> Void)?, on queue: DispatchQueue)
+
     /// This method is expected to destroy all persisted data. A notification of type `StorageManagerDidResetStorage` should get
     /// posted.
     ///

Storage/StorageTests/CoreData/CoreDataManagerTests.swift

@@ -41,20 +41,6 @@ final class CoreDataManagerTests: XCTestCase {
         XCTAssertEqual(manager.viewStorage as? NSManagedObjectContext, manager.persistentContainer.viewContext)
     }
 
-    /// Verifies that performBackgroundTask effectively runs received closure in BG.
-    ///
-    func test_performBackgroundTask_effectively_runs_received_closure_in_background_thread() {
-        let manager = CoreDataManager(name: storageIdentifier, crashLogger: MockCrashLogger())
-        let expectation = self.expectation(description: "Background")
-
-        manager.performBackgroundTask { (_) in
-            XCTAssertFalse(Thread.isMainThread)
-            expectation.fulfill()
-        }
-
-        wait(for: [expectation], timeout: Constants.expectationTimeout)
-    }
-
     /// Verifies that derived context is instantiated correctly.
     ///
     func test_derived_storage_is_instantiated_correctly() {
@@ -65,7 +51,7 @@ final class CoreDataManagerTests: XCTestCase {
         XCTAssertNotNil(viewContext)
         XCTAssertNotNil(derivedContext)
         XCTAssertNotEqual(derivedContext, viewContext)
-        XCTAssertEqual(derivedContext?.parent, viewContext)
+        XCTAssertNil(derivedContext?.parent)
     }
 
     func test_resetting_CoreData_deletes_preexisting_objects() throws {
@@ -84,7 +70,7 @@ final class CoreDataManagerTests: XCTestCase {
         XCTAssertEqual(viewContext.countObjects(ofType: ShippingLine.self), 0)
     }
 
-    func test_saving_derived_storage_while_resetting_CoreData_still_saves_data() throws {
+    func test_saving_derived_storage_while_resetting_CoreData_does_not_save_data() throws {
         // Arrange
         let manager = CoreDataManager(name: storageIdentifier, crashLogger: MockCrashLogger())
         manager.reset()
@@ -97,14 +83,15 @@ final class CoreDataManagerTests: XCTestCase {
                 _ = derivedContext.insertNewObject(ofType: ShippingLine.self)
             }
             manager.saveDerivedType(derivedStorage: derivedContext, {
+                XCTAssertEqual(viewContext.countObjects(ofType: ShippingLine.self), 1)
                 expectation.fulfill()
             })
             manager.reset()
             expectation.fulfill()
         }
 
         // Assert
-        XCTAssertEqual(viewContext.countObjects(ofType: ShippingLine.self), 1)
+        XCTAssertEqual(viewContext.countObjects(ofType: ShippingLine.self), 0)
     }
 
     func test_when_the_model_is_incompatible_then_it_recovers_and_recreates_the_database() throws {

WooCommerce/Classes/AppDelegate.swift

@@ -40,6 +40,9 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
     ///
     private var appCoordinator: AppCoordinator?
 
+    /// Initializes storage manager along with AppDelegate
+    private let storageManager = ServiceLocator.storageManager
+
     /// Tab Bar Controller
     ///
     var tabBarController: MainTabBarController? {
@@ -191,7 +194,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
 
             // When the app is put into an incative state, it's important to ensure that any pending changes to
             // Core Data context are saved to avoid data loss.
-            ServiceLocator.storageManager.viewStorage.saveIfNeeded()
+            storageManager.viewStorage.saveIfNeeded()
         }
     }
 
@@ -231,7 +234,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
 
         // When the app is put into the background, it's important to ensure that any pending changes to
         // Core Data context are saved to avoid data loss.
-        ServiceLocator.storageManager.viewStorage.saveIfNeeded()
+        storageManager.viewStorage.saveIfNeeded()
     }
 
     func applicationWillEnterForeground(_ application: UIApplication) {
@@ -253,7 +256,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
         NotificationCenter.default.post(name: .applicationTerminating, object: nil)
 
         // Save changes in the application's managed object context before the application terminates.
-        ServiceLocator.storageManager.viewStorage.saveIfNeeded()
+        storageManager.viewStorage.saveIfNeeded()
     }
 
     func application(_ application: UIApplication,
@@ -459,7 +462,7 @@ private extension AppDelegate {
         }
 
         if ProcessConfiguration.shouldUseScreenshotsNetworkLayer {
-            ServiceLocator.setStores(ScreenshotStoresManager(storageManager: ServiceLocator.storageManager))
+            ServiceLocator.setStores(ScreenshotStoresManager(storageManager: storageManager))
         }
 
         if ProcessConfiguration.shouldSimulatePushNotification {

WooCommerce/Classes/Model/MarkOrderAsReadUseCase.swift

@@ -25,6 +25,7 @@ struct MarkOrderAsReadUseCase {
     /// and then we compare local `orderID` with the one from remote `Note`.
     /// If they match we mark it as read.
     /// Returns syncronized note if marking was successful and error if some error happened
+    @MainActor
     static func markOrderNoteAsReadIfNeeded(stores: StoresManager, noteID: Int64, orderID: Int) async -> Result<Note, Error> {
         let syncronizedNoteResult: Result<Note, Error> = await withCheckedContinuation { continuation in
             let action = NotificationAction.synchronizeNotification(noteID: noteID) { syncronizedNote, error in

WooCommerce/WooCommerce.xcodeproj/xcshareddata/xcschemes/WooCommerce.xcscheme

@@ -123,6 +123,10 @@
             argument = "mocked-network-layer"
             isEnabled = "NO">
          </CommandLineArgument>
+         <CommandLineArgument
+            argument = "-enforce-core-data-write-in-background"
+            isEnabled = "NO">
+         </CommandLineArgument>
          <CommandLineArgument
             argument = "-com.apple.CoreData.ConcurrencyDebug 1"
             isEnabled = "YES">

Yosemite/Yosemite/Model/Storage/Product+ReadOnlyConvertible.swift

@@ -124,8 +124,8 @@ extension Storage.Product: ReadOnlyConvertible {
                        name: name,
                        slug: slug,
                        permalink: permalink,
-                       date: date,
-                       dateCreated: dateCreated,
+                       date: date ?? Date(),
+                       dateCreated: dateCreated ?? Date(),
                        dateModified: dateModified,
                        dateOnSaleStart: dateOnSaleStart,
                        dateOnSaleEnd: dateOnSaleEnd,