tech(store): Ignore multiple changes to same entity while storing/upd…

…ating (#55)
pjechris · Aug 8, 2023 · 8cd62be · 8cd62be
1 parent 8779ade
commit 8cd62be
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 25 deletions.
diff --git a/Sources/CohesionKit/Identity/IdentityStore.swift b/Sources/CohesionKit/Identity/IdentityStore.swift
@@ -154,6 +154,10 @@ public class IdentityMap {
             registry.enqueueChange(for: $0)
         }]
 
+        guard !registry.hasPendingChange(for: node) else {
+            return node
+        }
+
         do {
             try node.updateEntity(entity, modifiedAt: modifiedAt)
             logger?.didStore(T.self, id: entity.id)
@@ -170,8 +174,9 @@ public class IdentityMap {
             registry.enqueueChange(for: $0)
         }]
 
-        // disable changes while doing the entity update
-        node.applyChildrenChanges = false
+        guard !registry.hasPendingChange(for: node) else {
+            return node
+        }
 
         // clear all children to avoid a removed child to be kept as child
         node.removeAllChildren()
@@ -180,8 +185,6 @@ public class IdentityMap {
             keyPathContainer.accept(node, entity, modifiedAt, storeVisitor)
         }
 
-        node.applyChildrenChanges = true
-
         do {
             try node.updateEntity(entity, modifiedAt: modifiedAt)
             logger?.didStore(T.self, id: entity.id)

diff --git a/Sources/CohesionKit/Storage/EntityNode.swift b/Sources/CohesionKit/Storage/EntityNode.swift
@@ -17,7 +17,6 @@ class EntityNode<T>: AnyEntityNode {
         let node: AnyEntityNode
     }
 
-    var applyChildrenChanges = true
     var value: Any { ref.value }
 
     /// An observable entity reference
@@ -85,10 +84,6 @@ class EntityNode<T>: AnyEntityNode {
         }
 
         let subscription = childNode.ref.addObserver { [unowned self] newValue in
-            guard self.applyChildrenChanges else {
-                return
-            }
-
             update(&self.ref.value, newValue)
             self.onChange?(self)
         }

diff --git a/Tests/CohesionKitTests/IdentityMapTests.swift b/Tests/CohesionKitTests/IdentityMapTests.swift
@@ -20,34 +20,41 @@ class IdentityMapTests: XCTestCase {
         }
     }
 
-    func test_nodeStoreAggregate_nestedOptionalReplacedByNil_previousOptionalIdentityChange_nestedOptionalRemainsNil() {
+    func test_storeAggregate_nestedEntityReplacedByNil_entityIsUpdated_aggregateEntityRemainsNil() {
         let identityMap = IdentityMap()
-        var nestedOptional = OptionalNodeFixture(id: 1)
+        let nestedOptional = OptionalNodeFixture(id: 1)
         var root = RootFixture(id: 1, primitive: "", singleNode: SingleNodeFixture(id: 1), optional: nestedOptional, listNodes: [])
-        var node: EntityNode<RootFixture> = identityMap.nodeStore(entity: root, modifiedAt: Date().stamp)
 
-        root.optional = nil
-        node = identityMap.nodeStore(entity: root, modifiedAt: Date().stamp)
+        withExtendedLifetime(identityMap.store(entity: root)) {
+            root.optional = nil
 
-        nestedOptional.properties = ["bla": "blob"]
-        _ = identityMap.store(entity: nestedOptional)
+            _ = identityMap.store(entity: root)
+            _ = identityMap.store(entity: nestedOptional)
 
-        XCTAssertNil((node.value as! RootFixture).optional)
+            XCTAssertNotNil(identityMap.find(RootFixture.self, id: 1))
+            XCTAssertNil(identityMap.find(RootFixture.self, id: 1)!.value.optional)
+        }
     }
 
-    func test_nodeStoreAggregate_nestedArrayHasEntityRemoved_removedEntityChange_aggregateArrayNotChanged() {
+    /// check that removed relations do not trigger an update
+    func test_storeAggregate_removeEntityFromNestedArray_removedEntityChange_aggregateArrayNotChanged() {
         let identityMap = IdentityMap()
-        var nestedArray: [ListNodeFixture] = [ListNodeFixture(id: 1), ListNodeFixture(id: 2)]
+        var entityToRemove = ListNodeFixture(id: 2)
+        let nestedArray: [ListNodeFixture] = [entityToRemove, ListNodeFixture(id: 1)]
         var root = RootFixture(id: 1, primitive: "", singleNode: SingleNodeFixture(id: 1), optional: OptionalNodeFixture(id: 1), listNodes: nestedArray)
-        var node: EntityNode<RootFixture> = identityMap.nodeStore(entity: root, modifiedAt: Date().stamp)
 
-        nestedArray.removeLast()
-        root.listNodes = nestedArray
-        node = identityMap.nodeStore(entity: root, modifiedAt: Date().stamp)
+        withExtendedLifetime(identityMap.store(entity: root)) {
+            root.listNodes = Array(nestedArray[1...])
+            entityToRemove.key = "changed"
+
+            _ = identityMap.store(entity: root)
+            _ = identityMap.store(entity: entityToRemove)
 
-        _ = identityMap.store(entity: ListNodeFixture(id: 2, key: "changed"))
+            let storedRoot = identityMap.find(RootFixture.self, id: 1)!.value
 
-        XCTAssertEqual((node.value as! RootFixture).listNodes, nestedArray)
+            XCTAssertFalse(storedRoot.listNodes.contains(entityToRemove))
+            XCTAssertFalse(storedRoot.listNodes.map(\.id).contains(entityToRemove.id))
+        }
     }
 
     func test_storeAggregate_nestedWrapperChanged_aggregateIsUpdated() {
@@ -88,6 +95,19 @@ class IdentityMapTests: XCTestCase {
         }
     }
 
+    /// make sure when inserting multiple time the same entity that it actually gets inserted only once
+    func test_storeEntities_sameEntityPresentMultipleTimes_itIsInsertedOnce() {
+        let registry = ObserverRegistryStub(queue: .main)
+        let identityMap = IdentityMap(registry: registry)
+        let commonEntity = SingleNodeFixture(id: 1)
+        let root1 = RootFixture(id: 1, primitive: "", singleNode: commonEntity, optional: OptionalNodeFixture(id: 1), listNodes: [], enumWrapper: .single(SingleNodeFixture(id: 2)))
+        let root2 = RootFixture(id: 1, primitive: "", singleNode: commonEntity, optional: OptionalNodeFixture(id: 1), listNodes: [], enumWrapper: nil)
+
+        _ = identityMap.store(entities: [root1, root2])
+
+        XCTAssertEqual(registry.pendingChangeCount(for: commonEntity), 1)
+    }
+
     func test_storeIdentifiable_entityIsInsertedForThe1stTime_loggerIsCalled() {
         let logger = LoggerMock()
         let identityMap = IdentityMap(logger: logger)

diff --git a/Tests/CohesionKitTests/Observer/Stub/ObserverRegistryStub.swift b/Tests/CohesionKitTests/Observer/Stub/ObserverRegistryStub.swift
@@ -3,6 +3,7 @@
 /// An ObserverRegistry stub: all methods have default behaviour. Some can be mocked and some can be tracked
 class ObserverRegistryStub: ObserverRegistry {
     var enqueueChangeCalled: (AnyHashable) -> Void = { _ in }
+    /// Enqueued changes. Not typed. A same change could be enqueue multiple times (for testing purposes!)
     private var pendingChangesStub: [Any] = []
 
 
@@ -15,4 +16,9 @@ class ObserverRegistryStub: ObserverRegistry {
     func hasPendingChange<T: Equatable>(for entity: T) -> Bool {
         pendingChangesStub.contains { ($0 as? EntityNode<T>)?.ref.value == entity }
     }
+
+    /// number of times change has been inserted for this entity
+    func pendingChangeCount<T: Equatable>(for entity: T) -> Int {
+        pendingChangesStub.filter { ($0 as? EntityNode<T>)?.ref.value == entity }.count
+    }
 }