[store] don't store same node/entity multiple times inside a single t…

…ransaction
pjechris · Aug 8, 2023 · d2275ac · d2275ac
1 parent 845aed0
commit d2275ac
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 16 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,6 +174,10 @@ public class IdentityMap {
             registry.enqueueChange(for: $0)
         }]
 
+        guard !registry.hasPendingChange(for: node) else {
+            return node
+        }
+
         // clear all children to avoid a removed child to be kept as child
         node.removeAllChildren()
 

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
+    }
 }