Fix invalid TreeChanges in concurrent Tree.Style

yorkie-team · Jan 25, 2024 · 6b94ba2 · 6b94ba2
1 parent add52ad
commit 6b94ba2
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 36 deletions.
diff --git a/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt b/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt
@@ -17,7 +17,6 @@ import dev.yorkie.document.json.TreeBuilder.text
 import dev.yorkie.document.operation.OperationInfo
 import dev.yorkie.document.operation.OperationInfo.SetOpInfo
 import dev.yorkie.document.operation.OperationInfo.TreeEditOpInfo
-import dev.yorkie.document.operation.OperationInfo.TreeStyleOpInfo
 import kotlin.test.assertEquals
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.CoroutineStart
@@ -1721,10 +1720,7 @@ class JsonTreeTest {
                         "$.t",
                     ),
                     // client1 changed attributes on path [0]
-                    /* FIXME: TreeStyleOpInfo here should have not been emitted, as path [0] is already deleted.
-                     This causes users who depend on OperationInfo event streams to not be consistent with the actual document.
-                     And the resulting document would be as below:
-
+                    /* assert style changes on already deleted path is not applied
                      {
                         "t": {
                             "type": "root",
@@ -1734,20 +1730,13 @@ class JsonTreeTest {
                                     "children": [],
                                     "attributes": {
                                         "id": "2",
-                                        "value": "changed"
+                                        "value": "init"
                                     }
                                 }
                             ]
                         }
                      }
                      */
-                    TreeStyleOpInfo(
-                        0,
-                        0,
-                        listOf(0),
-                        mapOf("value" to "changed"),
-                        "$.t",
-                    ),
                 ),
                 document2Ops,
             )

diff --git a/yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt b/yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt
@@ -58,32 +58,31 @@ internal class CrdtTree(
     ): List<TreeChange> {
         val (fromParent, fromLeft) = findNodesAndSplitText(range.first, executedAt)
         val (toParent, toLeft) = findNodesAndSplitText(range.second, executedAt)
-        val changes = listOf(
-            TreeChange(
-                type = TreeChangeType.Style,
-                from = toIndex(fromParent, fromLeft),
-                to = toIndex(toParent, toLeft),
-                fromPath = toPath(fromParent, fromLeft),
-                toPath = toPath(toParent, toLeft),
-                actorID = executedAt.actorID,
-                attributes = attributes,
-            ),
-        )
-
-        traverseInPosRange(
-            fromParent = fromParent,
-            fromLeft = fromLeft,
-            toParent = toParent,
-            toLeft = toLeft,
-        ) { (node, _), _ ->
-            if (!node.isRemoved && attributes != null && !node.isText) {
-                attributes.forEach { (key, value) ->
-                    node.setAttribute(key, value, executedAt)
+        return buildList {
+            traverseInPosRange(
+                fromParent = fromParent,
+                fromLeft = fromLeft,
+                toParent = toParent,
+                toLeft = toLeft,
+            ) { (node, _), _ ->
+                if (!node.isRemoved && attributes != null && !node.isText) {
+                    attributes.forEach { (key, value) ->
+                        node.setAttribute(key, value, executedAt)
+                    }
+                    add(
+                        TreeChange(
+                            type = TreeChangeType.Style,
+                            from = toIndex(fromParent, fromLeft),
+                            to = toIndex(toParent, toLeft),
+                            fromPath = toPath(fromParent, fromLeft),
+                            toPath = toPath(toParent, toLeft),
+                            actorID = executedAt.actorID,
+                            attributes = attributes,
+                        ),
+                    )
                 }
             }
         }
-
-        return changes
     }
 
     private fun toPath(parentNode: CrdtTreeNode, leftSiblingNode: CrdtTreeNode): List<Int> {