Fix incorrect indexes in TreeChange

docker/docker-compose-ci.yml

@@ -2,7 +2,7 @@ version: '3.3'
 
 services:
   yorkie:
-    image: 'yorkieteam/yorkie:0.4.19'
+    image: 'yorkieteam/yorkie:0.4.22'
     container_name: 'yorkie'
     command: ['server', '--mongo-connection-uri', 'mongodb://mongo:27017']
     restart: always

docker/docker-compose.yml

@@ -2,7 +2,7 @@ version: '3.3'
 
 services:
   yorkie:
-    image: 'yorkieteam/yorkie:0.4.20'
+    image: 'yorkieteam/yorkie:0.4.22'
     container_name: 'yorkie'
     command: ['server', '--enable-pprof']
     restart: always

yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeConcurrencyTest.kt

@@ -313,10 +313,10 @@ class JsonTreeConcurrencyTest {
                 }
             }
             val initialXml =
-                "<r><p color=\"red\">a</p><p color=\"red\">b</p><p color=\"red\">c</p></r>"
+                """<r><p color="red">a</p><p color="red">b</p><p color="red">c</p></r>"""
             val content = element("p") {
                 text { "d" }
-                attrs { mapOf("italic" to "true") }
+                attrs { mapOf("italic" to "true", "color" to "blue") }
             }
 
             val ranges = listOf(
@@ -387,7 +387,7 @@ class JsonTreeConcurrencyTest {
                     StyleOpCode.StyleRemove,
                     "color",
                     "",
-                    "remove-bold",
+                    "remove-color",
                 ),
                 StyleOperationType(
                     RangeSelector.RangeAll,

yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt

@@ -37,6 +37,7 @@ import kotlinx.coroutines.flow.filterIsInstance
 import kotlinx.coroutines.flow.flatMapConcat
 import kotlinx.coroutines.flow.map
 import kotlinx.coroutines.flow.onEach
+import kotlinx.coroutines.joinAll
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.withTimeout
 import org.junit.Test
@@ -153,7 +154,7 @@ class JsonTreeTest {
                 Updater(c2, d2),
             )
             assertTreesXmlEquals(
-                """<doc><p italic="true" bold="true">hello</p></doc>""",
+                """<doc><p bold="true" italic="true">hello</p></doc>""",
                 d1,
                 d2,
             )
@@ -2159,6 +2160,146 @@ class JsonTreeTest {
         }
     }
 
+    // Tests for concurrent tree modifications causing incorrect remote event paths.
+    @Test
+    fun test_concurrent_tree_style_and_insertion() {
+        withTwoClientsAndDocuments(syncMode = Client.SyncMode.Manual) { c1, c2, d1, d2, _ ->
+            updateAndSync(
+                Updater(c1, d1) { root, _ ->
+                    root.setNewTree(
+                        "t",
+                        element("doc") {
+                            element("p")
+                        },
+                    )
+                },
+                Updater(c2, d2),
+            )
+            assertTreesXmlEquals("""<doc><p></p></doc>""", d1, d2)
+
+            val d1Events = mutableListOf<TreeEditOpInfo>()
+            val d2Events = mutableListOf<TreeStyleOpInfo>()
+
+            val collectJob = launch(start = CoroutineStart.UNDISPATCHED) {
+                launch(start = CoroutineStart.UNDISPATCHED) {
+                    d1.events.filterIsInstance<RemoteChange>()
+                        .map {
+                            it.changeInfo.operations.filterIsInstance<TreeEditOpInfo>()
+                        }
+                        .collect(d1Events::addAll)
+                }
+                launch(start = CoroutineStart.UNDISPATCHED) {
+                    d2.events.filterIsInstance<RemoteChange>()
+                        .map {
+                            it.changeInfo.operations.filterIsInstance<TreeStyleOpInfo>()
+                        }
+                        .collect(d2Events::addAll)
+                }
+            }
+
+            listOf(
+                launch {
+                    repeat(10) {
+                        d1.updateAsync { root, _ ->
+                            root.rootTree().style(0, 1, mapOf("style" to it.toString()))
+                        }.await()
+                    }
+                },
+                launch {
+                    d2.updateAsync { root, _ ->
+                        root.rootTree().edit(0, 0, element("p2"))
+                    }.await()
+                },
+            ).joinAll()
+
+            c1.syncAsync().await()
+            c2.syncAsync().await()
+            c1.syncAsync().await()
+
+            // the resulting trees are the same.
+            assertEquals(d1.getRoot().rootTree().toXml(), d2.getRoot().rootTree().toXml())
+
+            // insert event is sent with path [0]
+            assertEquals(listOf(0), d1Events.single().fromPath)
+
+            d2Events.forEach {
+                assertEquals(listOf(1), it.fromPath)
+            }
+
+            collectJob.cancel()
+        }
+    }
+
+    @Test
+    fun test_concurrent_tree_remove_style_and_insertion() {
+        withTwoClientsAndDocuments(syncMode = Client.SyncMode.Manual) { c1, c2, d1, d2, _ ->
+            updateAndSync(
+                Updater(c1, d1) { root, _ ->
+                    root.setNewTree(
+                        "t",
+                        element("doc") {
+                            element("p") {
+                                attrs { mapOf("key" to "a") }
+                            }
+                        },
+                    )
+                },
+                Updater(c2, d2),
+            )
+            assertTreesXmlEquals("""<doc><p key="a"></p></doc>""", d1, d2)
+
+            val d1Events = mutableListOf<TreeEditOpInfo>()
+            val d2Events = mutableListOf<TreeStyleOpInfo>()
+
+            val collectJob = launch(start = CoroutineStart.UNDISPATCHED) {
+                launch(start = CoroutineStart.UNDISPATCHED) {
+                    d1.events.filterIsInstance<RemoteChange>()
+                        .map {
+                            it.changeInfo.operations.filterIsInstance<TreeEditOpInfo>()
+                        }
+                        .collect(d1Events::addAll)
+                }
+                launch(start = CoroutineStart.UNDISPATCHED) {
+                    d2.events.filterIsInstance<RemoteChange>()
+                        .map {
+                            it.changeInfo.operations.filterIsInstance<TreeStyleOpInfo>()
+                        }
+                        .collect(d2Events::addAll)
+                }
+            }
+
+            listOf(
+                launch {
+                    repeat(10) {
+                        d1.updateAsync { root, _ ->
+                            root.rootTree().removeStyle(0, 1, listOf("key"))
+                        }.await()
+                    }
+                },
+                launch {
+                    d2.updateAsync { root, _ ->
+                        root.rootTree().edit(0, 0, element("p2"))
+                    }.await()
+                },
+            ).joinAll()
+
+            c1.syncAsync().await()
+            c2.syncAsync().await()
+            c1.syncAsync().await()
+
+            // the resulting trees are the same.
+            assertEquals(d1.getRoot().rootTree().toXml(), d2.getRoot().rootTree().toXml())
+
+            // insert event is sent with path [0]
+            assertEquals(listOf(0), d1Events.single().fromPath)
+            d2Events.forEach {
+                assertEquals(listOf(1), it.fromPath)
+            }
+
+            collectJob.cancel()
+        }
+    }
+
     companion object {
 
         fun JsonObject.rootTree() = getAs<JsonTree>("t")

yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt

@@ -23,6 +23,8 @@ public typealias TreePosRange = Pair<CrdtTreePos, CrdtTreePos>
 
 internal typealias CrdtTreeToken = TreeToken<CrdtTreeNode>
 
+internal typealias TreeNodePair = Pair<CrdtTreeNode, CrdtTreeNode>
+
 internal data class CrdtTree(
     val root: CrdtTreeNode,
     override val createdAt: TimeTicket,
@@ -93,6 +95,9 @@ internal data class CrdtTree(
                     createdAtMapByActor[actorID] = createdAt
                 }
 
+                val parentOfNode = requireNotNull(node.parent)
+                val previousNode = node.prevSibling ?: parentOfNode
+
                 val updatedAttrPairs = node.setAttributes(attributes, executedAt)
                 val affectedAttrs = updatedAttrPairs.fold(emptyMap<String, String>()) { acc, pair ->
                     val curr = pair.new
@@ -101,10 +106,10 @@ internal data class CrdtTree(
                 if (affectedAttrs.isNotEmpty()) {
                     TreeChange(
                         type = TreeChangeType.Style,
-                        from = toIndex(fromParent, fromLeft),
-                        to = toIndex(toParent, toLeft),
-                        fromPath = toPath(fromParent, fromLeft),
-                        toPath = toPath(toParent, toLeft),
+                        from = toIndex(parentOfNode, previousNode),
+                        to = toIndex(node, node),
+                        fromPath = toPath(parentOfNode, previousNode),
+                        toPath = toPath(node, node),
                         actorID = executedAt.actorID,
                         attributes = attributes.filterKeys { it in affectedAttrs },
                     ).let(changes::add)
@@ -423,31 +428,48 @@ internal data class CrdtTree(
         range: TreePosRange,
         attributeToRemove: List<String>,
         executedAt: TimeTicket,
+        maxCreatedAtMapByActor: Map<ActorID, TimeTicket>? = null,
     ): TreeOperationResult {
         val (fromParent, fromLeft) = findNodesAndSplitText(range.first, executedAt)
         val (toParent, toLeft) = findNodesAndSplitText(range.second, executedAt)
 
         val changes = mutableListOf<TreeChange>()
+        val createdAtMapByActor = mutableMapOf<ActorID, TimeTicket>()
         val gcPairs = mutableListOf<GCPair<RhtNode>>()
         traverseInPosRange(fromParent, fromLeft, toParent, toLeft) { (node, _), _ ->
-            if (!node.isRemoved && !node.isText && attributeToRemove.isNotEmpty()) {
+            val actorID = node.createdAt.actorID
+            val maxCreatedAt = maxCreatedAtMapByActor?.let {
+                maxCreatedAtMapByActor[actorID] ?: InitialTimeTicket
+            } ?: MaxTimeTicket
+
+            if (node.canStyle(executedAt, maxCreatedAt) && attributeToRemove.isNotEmpty()) {
+                val max = createdAtMapByActor[actorID]
+                val createdAt = node.createdAt
+                if (max < createdAt) {
+                    createdAtMapByActor[actorID] = createdAt
+                }
+
                 attributeToRemove.forEach { key ->
                     node.removeAttribute(key, executedAt)
                         .map { rhtNode -> GCPair(node, rhtNode) }
                         .let(gcPairs::addAll)
                 }
+
+                val parentOfNode = requireNotNull(node.parent)
+                val previousNode = node.prevSibling ?: parentOfNode
+
                 TreeChange(
                     type = TreeChangeType.RemoveStyle,
-                    from = toIndex(fromParent, fromLeft),
-                    to = toIndex(toParent, toLeft),
-                    fromPath = toPath(fromParent, fromLeft),
-                    toPath = toPath(toParent, toLeft),
+                    from = toIndex(parentOfNode, previousNode),
+                    to = toIndex(node, node),
+                    fromPath = toPath(parentOfNode, previousNode),
+                    toPath = toPath(node, node),
                     actorID = executedAt.actorID,
                     attributesToRemove = attributeToRemove,
                 ).let(changes::add)
             }
         }
-        return TreeOperationResult(changes, gcPairs)
+        return TreeOperationResult(changes, gcPairs, createdAtMapByActor)
     }
 
     private fun traverseInPosRange(
@@ -475,12 +497,9 @@ internal data class CrdtTree(
      * If [executedAt] is given, then it is used to find the appropriate left node
      * for concurrent insertion.
      */
-    fun findNodesAndSplitText(
-        pos: CrdtTreePos,
-        executedAt: TimeTicket? = null,
-    ): Pair<CrdtTreeNode, CrdtTreeNode> {
+    fun findNodesAndSplitText(pos: CrdtTreePos, executedAt: TimeTicket? = null): TreeNodePair {
         // 01. Find the parent and left sibling node of the given position.
-        val (parent, leftSibling) = pos.toTreeNodes(this)
+        val (parent, leftSibling) = pos.toTreeNodePair(this)
 
         // 02. Determine whether the position is left-most and the exact parent
         // in the current tree.
@@ -661,7 +680,7 @@ internal data class CrdtTree(
     /**
      * Converts the pos to parent and left sibling nodes.
      */
-    private fun CrdtTreePos.toTreeNodes(tree: CrdtTree): Pair<CrdtTreeNode, CrdtTreeNode> {
+    private fun CrdtTreePos.toTreeNodePair(tree: CrdtTree): TreeNodePair {
         val parentNode = tree.findFloorNode(parentID)
         val leftNode = tree.findFloorNode(leftSiblingID)
         require(parentNode != null && leftNode != null) {
@@ -888,7 +907,7 @@ internal data class CrdtTreeNode private constructor(
         if (isText) {
             return false
         }
-        return createdAt <= maxCreatedAt && (removedAt == null || removedAt < executedAt)
+        return createdAt <= maxCreatedAt && removedAt < executedAt
     }
 
     override fun delete(node: RhtNode) {

yorkie/src/main/kotlin/dev/yorkie/document/crdt/Rht.kt

@@ -96,6 +96,7 @@ internal class Rht : Collection<RhtNode> {
      */
     fun toXml(): String {
         return nodeMapByKey.filterValues { !it.isRemoved }.entries
+            .sortedBy { it.key }
             .joinToString(" ") { (key, node) ->
                 "$key=\"${node.value}\""
             }

yorkie/src/main/kotlin/dev/yorkie/document/crdt/TreeInfo.kt

@@ -81,5 +81,5 @@ internal enum class TreeChangeType {
 internal data class TreeOperationResult(
     val changes: List<TreeChange>,
     val gcPairs: List<GCPair<*>> = emptyList(),
-    val maxCreatedAtMap: Map<ActorID, TimeTicket> = emptyMap(),
+    val maxCreatedAtMap: Map<ActorID, TimeTicket>,
 )

yorkie/src/main/kotlin/dev/yorkie/document/json/JsonTree.kt

@@ -92,14 +92,20 @@ public class JsonTree internal constructor(
         val fromPos = target.findPos(fromIndex)
         val toPos = target.findPos(toIndex)
         val executedAt = context.issueTimeTicket()
-        target.removeStyle(fromPos to toPos, attributesToRemove, executedAt)
+        val (_, gcPairs, maxCreatedAtMapByActor) = target.removeStyle(
+            fromPos to toPos,
+            attributesToRemove,
+            executedAt,
+        )
+        gcPairs.forEach(context::registerGCPair)
 
         context.push(
             TreeStyleOperation(
                 target.createdAt,
                 fromPos,
                 toPos,
                 executedAt,
+                maxCreatedAtMapByActor,
                 attributesToRemove = attributesToRemove,
             ),
         )

yorkie/src/main/kotlin/dev/yorkie/document/operation/OperationInfo.kt

@@ -92,6 +92,7 @@ public sealed class OperationInfo {
         val from: Int,
         val to: Int,
         val fromPath: List<Int>,
+        val toPath: List<Int>,
         val attributes: Map<String, String> = emptyMap(),
         val attributesToRemove: List<String> = emptyList(),
         override var path: String = INITIAL_PATH,

yorkie/src/main/kotlin/dev/yorkie/document/operation/TreeStyleOperation.kt

@@ -47,6 +47,7 @@ internal data class TreeStyleOperation(
                         it.from,
                         it.to,
                         it.fromPath,
+                        it.toPath,
                         it.attributes.orEmpty(),
                         path = root.createPath(parentCreatedAt),
                     )
@@ -58,13 +59,15 @@ internal data class TreeStyleOperation(
                     fromPos to toPos,
                     attributesToRemove,
                     executedAt,
+                    maxCreatedAtMapByActor,
                 )
                 gcPairs.forEach(root::registerGCPair)
                 changes.map {
                     TreeStyleOpInfo(
                         it.from,
                         it.to,
                         it.fromPath,
+                        it.toPath,
                         attributesToRemove = it.attributesToRemove.orEmpty(),
                         path = root.createPath(parentCreatedAt),
                     )