Merge pull request #263 from modelix/fix/detect-changed-children-in-a…

…-node-if-a-child-node-changes-its-role fix(model-datastructure): detect changed children in a node if a child node changes its role
modelix · Oct 2, 2023 · 9b7de09 · 9b7de09
2 parents d65ffb6 + 3a51328
commit 9b7de09
Show file tree

Hide file tree

Showing 3 changed files with 71 additions and 12 deletions.
diff --git a/model-datastructure/src/commonMain/kotlin/org/modelix/model/lazy/CLTree.kt b/model-datastructure/src/commonMain/kotlin/org/modelix/model/lazy/CLTree.kt
@@ -424,6 +424,12 @@ class CLTree : ITree, IBulkTree {
         nodesMap!!.visitChanges(
             oldVersion.nodesMap,
             object : CPHamtNode.IChangeVisitor {
+                private val childrenChangeEvents = HashSet<Pair<Long, String?>>()
+
+                private fun notifyChildrenChange(parent: Long, role: String?) {
+                    if (childrenChangeEvents.add(parent to role)) visitor.childrenChanged(parent, role)
+                }
+
                 override fun visitChangesOnly(): Boolean {
                     return changesOnly
                 }
@@ -450,8 +456,12 @@ class CLTree : ITree, IBulkTree {
                             if (oldElement!!::class != newElement!!::class) {
                                 throw RuntimeException("Unsupported type change of node " + key + "from " + oldElement::class.simpleName + " to " + newElement::class.simpleName)
                             }
-                            if (oldElement.parentId != newElement.parentId || oldElement.roleInParent != newElement.roleInParent) {
+                            if (oldElement.parentId != newElement.parentId) {
+                                visitor.containmentChanged(key)
+                            } else if (oldElement.roleInParent != newElement.roleInParent) {
                                 visitor.containmentChanged(key)
+                                notifyChildrenChange(oldElement.parentId, oldElement.roleInParent)
+                                notifyChildrenChange(newElement.parentId, newElement.roleInParent)
                             }
                             oldElement.propertyRoles.asSequence()
                                 .plus(newElement.propertyRoles.asSequence())
@@ -486,7 +496,7 @@ class CLTree : ITree, IBulkTree {
                                     val oldValues = oldChildrenInRole?.map { it.id }
                                     val newValues = newChildrenInRole?.map { it.id }
                                     if (oldValues != newValues) {
-                                        visitor.childrenChanged(newElement.id, role)
+                                        notifyChildrenChange(newElement.id, role)
                                     }
                                 }
                             }

diff --git a/model-datastructure/src/commonMain/kotlin/org/modelix/model/persistent/CPHamtNode.kt b/model-datastructure/src/commonMain/kotlin/org/modelix/model/persistent/CPHamtNode.kt
@@ -23,6 +23,9 @@ import org.modelix.model.persistent.SerializationUtil.intFromHex
 import org.modelix.model.persistent.SerializationUtil.longFromHex
 import kotlin.jvm.JvmStatic
 
+/**
+ * Implementation of a hash array mapped trie.
+ */
 abstract class CPHamtNode : IKVValue {
     override var isWritten: Boolean = false
 

diff --git a/model-datastructure/src/commonTest/kotlin/DiffTest.kt b/model-datastructure/src/commonTest/kotlin/DiffTest.kt
@@ -1,12 +1,3 @@
-import org.modelix.model.api.ConceptReference
-import org.modelix.model.api.ITree
-import org.modelix.model.api.NodeReference
-import org.modelix.model.lazy.CLTree
-import org.modelix.model.lazy.ObjectStoreCache
-import org.modelix.model.persistent.MapBaseStore
-import kotlin.test.Test
-import kotlin.test.assertEquals
-
 /*
  * Copyright (c) 2023.
  *
@@ -23,6 +14,16 @@ import kotlin.test.assertEquals
  * limitations under the License.
  */
 
+import org.modelix.model.api.ConceptReference
+import org.modelix.model.api.ITree
+import org.modelix.model.api.NodeReference
+import org.modelix.model.lazy.CLTree
+import org.modelix.model.lazy.ObjectStoreCache
+import org.modelix.model.persistent.MapBaseStore
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.fail
+
 class DiffTest {
 
     @Test
@@ -70,6 +71,43 @@ class DiffTest {
         }
     }
 
+    @Test
+    fun moveChildOutsideNode() {
+        runTest(
+            setOf(
+                TreeChangeCollector.ChildrenChangedEvent(ITree.ROOT_ID, "children1"),
+                TreeChangeCollector.ChildrenChangedEvent(200, "children1"),
+                TreeChangeCollector.ContainmentChangedEvent(100),
+            ),
+            {
+                it.addNewChild(ITree.ROOT_ID, "children1", -1, 100, null as ConceptReference?)
+                    .addNewChild(ITree.ROOT_ID, "children2", -1, 200, null as ConceptReference?)
+            },
+            {
+                it.moveChild(200, "children1", -1, 100)
+            },
+        )
+    }
+
+    @Test
+    fun moveChildInsideNode() {
+        runTest(
+            setOf(
+                TreeChangeCollector.ChildrenChangedEvent(ITree.ROOT_ID, "children1"),
+                TreeChangeCollector.ChildrenChangedEvent(ITree.ROOT_ID, "children2"),
+                TreeChangeCollector.ContainmentChangedEvent(100),
+            ),
+            {
+                it
+                    .addNewChild(ITree.ROOT_ID, "children1", -1, 100, null as ConceptReference?)
+                    .addNewChild(ITree.ROOT_ID, "children1", -1, 101, null as ConceptReference?)
+            },
+            {
+                it.moveChild(ITree.ROOT_ID, "children2", -1, 100)
+            },
+        )
+    }
+
     @Test
     fun removeChild() {
         runTest(
@@ -90,11 +128,19 @@ class DiffTest {
         runTest(expectedEvents, { it }, mutator)
     }
 
-    private fun runTest(expectedEvents: Set<TreeChangeCollector.ChangeEvent>, initialMutator: (ITree) -> ITree, mutator: (ITree) -> ITree) {
+    private fun runTest(
+        expectedEvents: Set<TreeChangeCollector.ChangeEvent>,
+        initialMutator: (ITree) -> ITree,
+        mutator: (ITree) -> ITree,
+    ) {
         val tree1 = initialMutator(CLTree.builder(ObjectStoreCache(MapBaseStore())).build())
         val tree2 = mutator(tree1)
         val collector = TreeChangeCollector()
         tree2.visitChanges(tree1, collector)
+        val duplicateEvents = collector.events.groupBy { it }.filter { it.value.size > 1 }.map { it.key }
+        if (duplicateEvents.isNotEmpty()) {
+            fail("duplicate events: $duplicateEvents")
+        }
 
         assertEquals(
             expectedEvents,