feat(bulk-model-sync): do not reorder unordered children during import

Child links that are unordered need to be handled specially when imported.
Such unordered children will never be moved in between other children.
Existing unordered children will not be reordered,
and new children will only be appended to the existing children.

Author: Oleksandr Dzhychko

bulk-model-sync-lib/src/commonMain/kotlin/org/modelix/model/sync/bulk/ModelImporter.kt

@@ -23,6 +23,7 @@ import org.modelix.model.api.INodeReference
 import org.modelix.model.api.INodeResolutionScope
 import org.modelix.model.api.SerializedNodeReference
 import org.modelix.model.api.getDescendants
+import org.modelix.model.api.isChildRoleOrdered
 import org.modelix.model.api.remove
 import org.modelix.model.data.ModelData
 import org.modelix.model.data.NodeData
@@ -118,15 +119,15 @@ class ModelImporter(private val root: INode, private val continueOnError: Boolea
         }
     }
 
-    private fun syncChildren(node: INode, data: NodeData, progressReporter: ProgressReporter) {
-        val allRoles = (data.children.map { it.role } + node.allChildren.map { it.roleInParent }).distinct()
+    private fun syncChildren(existingParent: INode, expectedParent: NodeData, progressReporter: ProgressReporter) {
+        val allRoles = (expectedParent.children.map { it.role } + existingParent.allChildren.map { it.roleInParent }).distinct()
         for (role in allRoles) {
-            val expectedNodes = data.children.filter { it.role == role }
-            val existingNodes = node.getChildren(role).toList()
+            val expectedNodes = expectedParent.children.filter { it.role == role }
+            val existingNodes = existingParent.getChildren(role).toList()
 
             // optimization that uses the bulk operation .addNewChildren
             if (existingNodes.isEmpty() && expectedNodes.all { originalIdToExisting[it.originalId()] == null }) {
-                node.addNewChildren(role, -1, expectedNodes.map { it.concept?.let { ConceptReference(it) } }).zip(expectedNodes).forEach { (newChild, expected) ->
+                existingParent.addNewChildren(role, -1, expectedNodes.map { it.concept?.let { ConceptReference(it) } }).zip(expectedNodes).forEach { (newChild, expected) ->
                     val expectedId = checkNotNull(expected.originalId()) { "Specified node '$expected' has no id" }
                     newChild.setPropertyValue(NodeData.idPropertyKey, expectedId)
                     originalIdToExisting[expectedId] = newChild
@@ -142,21 +143,38 @@ class ModelImporter(private val root: INode, private val continueOnError: Boolea
                 continue
             }
 
-            expectedNodes.forEachIndexed { index, expected ->
-                val nodeAtIndex = node.getChildren(role).toList().getOrNull(index)
+            val isOrdered = existingParent.isChildRoleOrdered(role)
+
+            expectedNodes.forEachIndexed { indexInImport, expected ->
+                val existingChildren = existingParent.getChildren(role).toList()
                 val expectedId = checkNotNull(expected.originalId()) { "Specified node '$expected' has no id" }
+                // newIndex is the index on which to import the expected child.
+                // It might be -1 if the child does not exist and should be added at the end.
+                val newIndex = if (isOrdered) {
+                    indexInImport
+                } else {
+                    // The `existingChildren` are only searched once for the expected element before changing.
+                    // Therefore, indexing existing children will not be more efficient than iterating once.
+                    // (For the moment, this is fine because as we expect unordered children to be the exception,
+                    // Reusable indexing would be possible if we switch from
+                    // a depth-first import to a breadth-first import.)
+                    existingChildren
+                        .indexOfFirst { existingChild -> existingChild.originalId() == expected.originalId() }
+                }
+                // existingChildren.getOrNull handles `-1` as needed by returning `null`.
+                val nodeAtIndex = existingChildren.getOrNull(newIndex)
                 val expectedConcept = expected.concept?.let { s -> ConceptReference(s) }
                 val childNode = if (nodeAtIndex?.originalId() != expectedId) {
                     val existingNode = originalIdToExisting[expectedId]
                     if (existingNode == null) {
-                        val newChild = node.addNewChild(role, index, expectedConcept)
+                        val newChild = existingParent.addNewChild(role, newIndex, expectedConcept)
                         newChild.setPropertyValue(NodeData.idPropertyKey, expectedId)
                         originalIdToExisting[expectedId] = newChild
                         newChild
                     } else {
                         // The existing child node is not only moved to a new index,
                         // it is potentially moved to a new parent and role.
-                        node.moveChild(role, index, existingNode)
+                        existingParent.moveChild(role, newIndex, existingNode)
                         // If the old parent and old role synchronized before the move operation,
                         // the existing child node would have been marked as to be deleted.
                         // Now that it is used, it should not be deleted.
@@ -171,10 +189,10 @@ class ModelImporter(private val root: INode, private val continueOnError: Boolea
                 syncNode(childNode, expected, progressReporter)
             }
 
-            // At this point, all n expected children for this role are created and correctly sorted.
-            // This means the first n children are correct.
-            // Any child beyond that is an unexpected child has to be marked for removal.
-            nodesToRemove += node.getChildren(role).drop(expectedNodes.size)
+            val expectedNodesIds = expectedNodes.map(NodeData::originalId).toSet()
+            // Do not use existingNodes, but call node.getChildren(role) because
+            // the recursive synchronization in the meantime already removed some nodes from node.getChildren(role).
+            nodesToRemove += existingParent.getChildren(role).filterNot { existingNode -> expectedNodesIds.contains(existingNode.originalId()) }
         }
     }