fix(DataStore): redundant local metadata query in ReconcileAndLocalSaveOperation (#1217)

* fix(DataStore): redundant local metadata query in ReconcileAndLocalSaveOperation

* fix(DataStore): remove unneeded mutationType parameter

Author: lawmicha

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/SubscriptionSync/ReconcileAndLocalSave/ReconcileAndLocalSaveOperation+Action.swift

@@ -23,7 +23,7 @@ extension ReconcileAndLocalSaveOperation {
 
         /// Operation has applied the incoming RemoteModel to the local database per the reconciled disposition. This
         /// could result in either a save to the local database, or a delete from the local database.
-        case applied(AppliedModel, existsLocally: Bool)
+        case applied(AppliedModel, mutationType: MutationEvent.MutationType)
 
         /// Operation dropped the remote model per the reconciled disposition.
         case dropped(modelName: String)

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/SubscriptionSync/ReconcileAndLocalSave/ReconcileAndLocalSaveOperation+Resolver.swift

@@ -34,8 +34,8 @@ extension ReconcileAndLocalSaveOperation {
             case (.notifyingDropped, .notified):
                 return .finished
 
-            case (.executing, .applied(let savedModel, let existsLocally)):
-                return .notifying(savedModel, existsLocally)
+            case (.executing, .applied(let savedModel, let mutationType)):
+                return .notifying(savedModel, mutationType)
 
             case (.notifying, .notified):
                 return .finished

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/SubscriptionSync/ReconcileAndLocalSave/ReconcileAndLocalSaveOperation+State.swift

@@ -27,7 +27,7 @@ extension ReconcileAndLocalSaveOperation {
         case notifyingDropped(String)
 
         /// Notifying listeners and callbacks of completion
-        case notifying(AppliedModel, Bool)
+        case notifying(AppliedModel, MutationEvent.MutationType)
 
         /// Operation has successfully completed
         case finished

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/SubscriptionSync/ReconcileAndLocalSave/ReconcileAndLocalSaveOperation.swift

@@ -100,8 +100,8 @@ class ReconcileAndLocalSaveOperation: AsynchronousOperation {
         case .notifyingDropped(let modelName):
             notifyDropped(modelName: modelName)
 
-        case .notifying(let savedModel, let existsLocally):
-            notify(savedModel: savedModel, existsLocally: existsLocally)
+        case .notifying(let savedModel, let mutationType):
+            notify(savedModel: savedModel, mutationType: mutationType)
 
         case .inError(let error):
             // Maybe we have to notify the Hub?
@@ -189,20 +189,18 @@ class ReconcileAndLocalSaveOperation: AsynchronousOperation {
     /// - dropped
     func execute(disposition: RemoteSyncReconciler.Disposition) {
         switch disposition {
-        case .applyRemoteModel(let remoteModel):
-            apply(remoteModel: remoteModel)
+        case .applyRemoteModel(let remoteModel, let mutationType):
+            apply(remoteModel: remoteModel, mutationType: mutationType)
         case .dropRemoteModel(let modelName):
             stateMachine.notify(action: .dropped(modelName: modelName))
-        case .error(let dataStoreError):
-            stateMachine.notify(action: .errored(dataStoreError))
         }
     }
 
     /// Execution method for the `applyRemoteModel` disposition. Does not notify directly, but delegates to save or
     /// delete methods, which eventually notify with:
     /// - applied
     /// - errored
-    private func apply(remoteModel: RemoteModel) {
+    private func apply(remoteModel: RemoteModel, mutationType: MutationEvent.MutationType) {
         if log.logLevel == .verbose {
             log.verbose("\(#function): remoteModel")
         }
@@ -218,15 +216,17 @@ class ReconcileAndLocalSaveOperation: AsynchronousOperation {
         }
 
         // TODO: Wrap this in a transaction
-        if remoteModel.syncMetadata.deleted {
+        if mutationType == .delete {
             saveDeleteMutation(storageAdapter: storageAdapter, remoteModel: remoteModel)
         } else {
-            saveCreateOrUpdateMutation(storageAdapter: storageAdapter, remoteModel: remoteModel)
+            saveCreateOrUpdateMutation(storageAdapter: storageAdapter,
+                                       remoteModel: remoteModel,
+                                       mutationType: mutationType)
         }
-
     }
 
-    private func saveDeleteMutation(storageAdapter: StorageEngineAdapter, remoteModel: RemoteModel) {
+    private func saveDeleteMutation(storageAdapter: StorageEngineAdapter,
+                                    remoteModel: RemoteModel) {
         log.verbose(#function)
 
         guard let modelType = ModelRegistry.modelType(from: modelSchema.name) else {
@@ -247,12 +247,16 @@ class ReconcileAndLocalSaveOperation: AsynchronousOperation {
                 let errorAction = Action.errored(dataStoreError)
                 self.stateMachine.notify(action: errorAction)
             case .success:
-                self.saveMetadata(storageAdapter: storageAdapter, inProcessModel: remoteModel)
+                self.saveMetadata(storageAdapter: storageAdapter,
+                                  inProcessModel: remoteModel,
+                                  mutationType: .delete)
             }
         }
     }
 
-    private func saveCreateOrUpdateMutation(storageAdapter: StorageEngineAdapter, remoteModel: RemoteModel) {
+    private func saveCreateOrUpdateMutation(storageAdapter: StorageEngineAdapter,
+                                            remoteModel: RemoteModel,
+                                            mutationType: MutationEvent.MutationType) {
         log.verbose(#function)
         storageAdapter.save(untypedModel: remoteModel.model.instance) { response in
             if self.log.logLevel == .debug {
@@ -271,25 +275,18 @@ class ReconcileAndLocalSaveOperation: AsynchronousOperation {
                     return
                 }
                 let inProcessModel = MutationSync(model: anyModel, syncMetadata: remoteModel.syncMetadata)
-                self.saveMetadata(storageAdapter: storageAdapter, inProcessModel: inProcessModel)
+                self.saveMetadata(storageAdapter: storageAdapter,
+                                  inProcessModel: inProcessModel,
+                                  mutationType: mutationType)
             }
         }
     }
 
     private func saveMetadata(storageAdapter: StorageEngineAdapter,
-                              inProcessModel: AppliedModel) {
+                              inProcessModel: AppliedModel,
+                              mutationType: MutationEvent.MutationType) {
         log.verbose(#function)
 
-        /// Do a local metadata query before saving to check if the `AppliedModel` is of `create` or
-        /// `update` MutationType from the perspective of the local store
-        let existsLocally: Bool
-        do {
-            let localMetadata = try storageAdapter.queryMutationSyncMetadata(for: remoteModel.model.id)
-            existsLocally = localMetadata != nil
-        } catch {
-            log.error("Failed to query for sync metadata")
-            return
-        }
         storageAdapter.save(remoteModel.syncMetadata, condition: nil) { result in
             if self.log.logLevel == .debug {
                 self.log.debug("save metadata: \(self.stopwatch.lap())s")
@@ -300,7 +297,7 @@ class ReconcileAndLocalSaveOperation: AsynchronousOperation {
                 self.stateMachine.notify(action: errorAction)
             case .success(let syncMetadata):
                 let appliedModel = MutationSync(model: inProcessModel.model, syncMetadata: syncMetadata)
-                self.stateMachine.notify(action: .applied(appliedModel, existsLocally: existsLocally))
+                self.stateMachine.notify(action: .applied(appliedModel, mutationType: mutationType))
             }
         }
     }
@@ -313,23 +310,14 @@ class ReconcileAndLocalSaveOperation: AsynchronousOperation {
     /// Responder method for `notifying`. Notify actions:
     /// - notified
     func notify(savedModel: AppliedModel,
-                existsLocally: Bool) {
+                mutationType: MutationEvent.MutationType) {
         log.verbose(#function)
 
         guard !isCancelled else {
             log.verbose("\(#function) - cancelled, aborting")
             return
         }
-
-        let mutationType: MutationEvent.MutationType
         let version = savedModel.syncMetadata.version
-        if savedModel.syncMetadata.deleted {
-            mutationType = .delete
-        } else if !existsLocally {
-            mutationType = .create
-        } else {
-            mutationType = .update
-        }
 
         // TODO: Dispatch/notify error if we can't erase to any model? Would imply an error in JSON decoding,
         // which shouldn't be possible this late in the process. Possibly notify global conflict/error handler?

AmplifyPlugins/DataStore/AWSDataStoreCategoryPlugin/Sync/SubscriptionSync/ReconcileAndLocalSave/RemoteSyncReconciler.swift

@@ -15,9 +15,8 @@ struct RemoteSyncReconciler {
     typealias SavedModel = ReconcileAndLocalSaveOperation.AppliedModel
 
     enum Disposition {
-        case applyRemoteModel(RemoteModel)
+        case applyRemoteModel(RemoteModel, MutationEvent.MutationType)
         case dropRemoteModel(String)
-        case error(DataStoreError)
     }
 
     static func reconcile(remoteModel: RemoteModel,
@@ -29,13 +28,21 @@ struct RemoteSyncReconciler {
         }
 
         guard let localMetadata = localMetadata else {
-            return .applyRemoteModel(remoteModel)
+            if remoteModel.syncMetadata.deleted {
+                return .dropRemoteModel(remoteModel.model.modelName)
+            } else {
+                return .applyRemoteModel(remoteModel, .create)
+            }
         }
 
         // Technically, we should never receive a subscription for a version we already have, but we'll be defensive
         // and make this check include the current version
         if remoteModel.syncMetadata.version >= localMetadata.version {
-            return .applyRemoteModel(remoteModel)
+            if remoteModel.syncMetadata.deleted {
+                return .applyRemoteModel(remoteModel, .delete)
+            } else {
+                return .applyRemoteModel(remoteModel, .update)
+            }
         }
 
         return .dropRemoteModel(remoteModel.model.modelName)

AmplifyPlugins/DataStore/AWSDataStoreCategoryPluginTests/Sync/SubscriptionSync/ReconcileAndLocalSaveOperationTests.swift

@@ -132,7 +132,7 @@ class ReconcileAndLocalSaveOperationTests: XCTestCase {
 
     func testReconcilingWithoutLocalModel() throws {
         let expect = expectation(description: "action .reconciled notified")
-        let expectedDisposition = RemoteSyncReconciler.Disposition.applyRemoteModel(anyPostMutationSync)
+        let expectedDisposition = RemoteSyncReconciler.Disposition.applyRemoteModel(anyPostMutationSync, .create)
         stateMachine.pushExpectActionCriteria { action in
             XCTAssertEqual(action, ReconcileAndLocalSaveOperation.Action.reconciled(expectedDisposition))
             expect.fulfill()
@@ -142,29 +142,28 @@ class ReconcileAndLocalSaveOperationTests: XCTestCase {
         waitForExpectations(timeout: 1)
     }
 
-    func testExecuteApplyRemoteModelThatDoesNotExistLocally() throws {
+    func testExecuteApplyRemoteModelCreate() throws {
         let expect = expectation(description: "action .execute applyRemoteModel")
-        let disposition = RemoteSyncReconciler.Disposition.applyRemoteModel(anyPostMutationSync)
+        let disposition = RemoteSyncReconciler.Disposition.applyRemoteModel(anyPostMutationSync, .create)
         storageAdapter.returnOnSave(dataStoreResult: .success(anyPostMutationSync.model))
         stateMachine.pushExpectActionCriteria { action in
             XCTAssertEqual(action, ReconcileAndLocalSaveOperation.Action.applied(self.anyPostMutationSync,
-                                                                                 existsLocally: false))
+                                                                                 mutationType: .create))
             expect.fulfill()
         }
 
         stateMachine.state = .executing(disposition)
         waitForExpectations(timeout: 1)
     }
 
-    func testExecuteApplyRemoteModelThatExistsLocally() throws {
+    func testExecuteApplyRemoteModelUpdate() throws {
         let expect = expectation(description: "action .execute applyRemoteModel")
-        let disposition = RemoteSyncReconciler.Disposition.applyRemoteModel(anyPostMutationSync)
+        let disposition = RemoteSyncReconciler.Disposition.applyRemoteModel(anyPostMutationSync, .update)
 
         storageAdapter.returnOnSave(dataStoreResult: .success(anyPostMutationSync.model))
-        storageAdapter.returnOnQueryMutationSyncMetadata(.some(anyPostMetadata))
         stateMachine.pushExpectActionCriteria { action in
             XCTAssertEqual(action, ReconcileAndLocalSaveOperation.Action.applied(self.anyPostMutationSync,
-                                                                                 existsLocally: true))
+                                                                                 mutationType: .update))
             expect.fulfill()
         }
         stateMachine.state = .executing(disposition)
@@ -174,7 +173,7 @@ class ReconcileAndLocalSaveOperationTests: XCTestCase {
 
     func testExecuteApplyRemoteModel_saveMutationFailed() throws {
         let expect = expectation(description: "action .execute error on save model")
-        let disposition = RemoteSyncReconciler.Disposition.applyRemoteModel(anyPostMutationSync)
+        let disposition = RemoteSyncReconciler.Disposition.applyRemoteModel(anyPostMutationSync, .create)
         let error = DataStoreError.invalidModelName("invModelName")
         storageAdapter.returnOnSave(dataStoreResult: .failure(error))
         stateMachine.pushExpectActionCriteria { action in
@@ -189,7 +188,7 @@ class ReconcileAndLocalSaveOperationTests: XCTestCase {
 
     func testExecuteApplyRemoteModel_saveMutationOK_MetadataFailed() throws {
         let expect = expectation(description: "action .execute error on save mutation")
-        let disposition = RemoteSyncReconciler.Disposition.applyRemoteModel(anyPostMutationSync)
+        let disposition = RemoteSyncReconciler.Disposition.applyRemoteModel(anyPostMutationSync, .create)
         let error = DataStoreError.invalidModelName("forceError")
         storageAdapter.returnOnSave(dataStoreResult: .success(anyPostMutationSync.model))
         storageAdapter.shouldReturnErrorOnSaveMetadata = true
@@ -204,11 +203,11 @@ class ReconcileAndLocalSaveOperationTests: XCTestCase {
 
     func testExecuteApplyRemoteModel_Delete() throws {
         let expect = expectation(description: "action .execute applyRemoteModel delete success case")
-        let disposition = RemoteSyncReconciler.Disposition.applyRemoteModel(anyPostDeletedMutationSync)
+        let disposition = RemoteSyncReconciler.Disposition.applyRemoteModel(anyPostDeletedMutationSync, .delete)
         storageAdapter.returnOnSave(dataStoreResult: .success(anyPostDeletedMutationSync.model))
         stateMachine.pushExpectActionCriteria { action in
             XCTAssertEqual(action, ReconcileAndLocalSaveOperation.Action.applied(self.anyPostDeletedMutationSync,
-                                                                                 existsLocally: false))
+                                                                                 mutationType: .delete))
             expect.fulfill()
         }
 
@@ -218,7 +217,7 @@ class ReconcileAndLocalSaveOperationTests: XCTestCase {
 
     func testExecuteApplyRemoteModel_Delete_saveMutationFailed() throws {
         let expect = expectation(description: "action .execute applyRemoteModel delete mutation error")
-        let disposition = RemoteSyncReconciler.Disposition.applyRemoteModel(anyPostDeletedMutationSync)
+        let disposition = RemoteSyncReconciler.Disposition.applyRemoteModel(anyPostDeletedMutationSync, .delete)
         let error = DataStoreError.invalidModelName("DelMutate")
         storageAdapter.shouldReturnErrorOnDeleteMutation = true
         stateMachine.pushExpectActionCriteria { action in
@@ -233,7 +232,7 @@ class ReconcileAndLocalSaveOperationTests: XCTestCase {
 
     func testExecuteApplyRemoteModel_Delete_saveMutationOK_saveMetadataFailed() throws {
         let expect = expectation(description: "action .execute applyRemoteModel delete metadata error")
-        let disposition = RemoteSyncReconciler.Disposition.applyRemoteModel(anyPostDeletedMutationSync)
+        let disposition = RemoteSyncReconciler.Disposition.applyRemoteModel(anyPostDeletedMutationSync, .delete)
         let error = DataStoreError.invalidModelName("forceError")
         storageAdapter.shouldReturnErrorOnSaveMetadata = true
         storageAdapter.returnOnSave(dataStoreResult: .failure(error))
@@ -259,20 +258,6 @@ class ReconcileAndLocalSaveOperationTests: XCTestCase {
         waitForExpectations(timeout: 1)
     }
 
-    func testExecuteErrorOnDisposition() throws {
-        let expect = expectation(description: "action .execute error")
-        let error = DataStoreError.invalidModelName("invModelName")
-        let disposition = RemoteSyncReconciler.Disposition.error(error)
-        stateMachine.pushExpectActionCriteria { action in
-            XCTAssertEqual(action, ReconcileAndLocalSaveOperation.Action.errored(error))
-            expect.fulfill()
-        }
-
-        stateMachine.state = .executing(disposition)
-
-        waitForExpectations(timeout: 1)
-    }
-
     func testNotifying() throws {
         let hubExpect = expectation(description: "Hub is notified")
         let notifyExpect = expectation(description: "action .notified notified")
@@ -286,7 +271,7 @@ class ReconcileAndLocalSaveOperationTests: XCTestCase {
             notifyExpect.fulfill()
         }
 
-        stateMachine.state = .notifying(anyPostMutationSync, false)
+        stateMachine.state = .notifying(anyPostMutationSync, .create)
 
         waitForExpectations(timeout: 1)
         Amplify.Hub.removeListener(hubListener)