From 8661963f0f261e0bf4c054dc12817b8bfe49159d Mon Sep 17 00:00:00 2001 From: Hyowoo Kim Date: Thu, 25 Jan 2024 15:53:49 +0900 Subject: [PATCH] Fix invalid TreeChanges in concurrent Tree.Style (#149) --- .../dev/yorkie/document/json/JsonTreeTest.kt | 191 +++++++++++++++++- .../dev/yorkie/document/crdt/CrdtTree.kt | 45 ++--- 2 files changed, 212 insertions(+), 24 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 index cc9d73bc5..f43e42662 100644 --- a/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt +++ b/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt @@ -10,8 +10,12 @@ import dev.yorkie.core.withTwoClientsAndDocuments import dev.yorkie.document.Document import dev.yorkie.document.Document.Event.LocalChange import dev.yorkie.document.Document.Event.RemoteChange +import dev.yorkie.document.json.JsonTree.ElementNode +import dev.yorkie.document.json.JsonTree.TreeNode import dev.yorkie.document.json.TreeBuilder.element 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 kotlin.test.assertEquals import kotlinx.coroutines.CoroutineScope @@ -1554,6 +1558,191 @@ class JsonTreeTest { } } + @Test + fun test_concurrently_deleting_and_styling_on_same_path() { + withTwoClientsAndDocuments( + realTimeSync = false, + ) { client1, client2, document1, document2, _ -> + val document1Ops = mutableListOf() + val document2Ops = mutableListOf() + + val collectJobs = listOf( + launch(start = CoroutineStart.UNDISPATCHED) { + document1.events.filterIsInstance() + .collect { + document1Ops.addAll(it.changeInfo.operations) + } + }, + launch(start = CoroutineStart.UNDISPATCHED) { + document2.events.filterIsInstance() + .collect { + document2Ops.addAll(it.changeInfo.operations) + } + }, + ) + + // client1 initializes tree + updateAndSync( + Updater(client1, document1) { root, _ -> + val tree = root.setNewTree("t") + tree.editByPath( + listOf(0), + listOf(0), + ElementNode("t", mapOf("id" to "1", "value" to "init")), + ElementNode("t", mapOf("id" to "2", "value" to "init")), + ) + }, + Updater(client2, document2), + ) + + /* assert both documents are synced right + { + "t": { + "type": "root", + "children": [ + { + "type": "t", + "children": [], + "attributes": { + "id": "1", + "value": "init" + } + }, + { + "type": "t", + "children": [], + "attributes": { + "id": "2", + "value": "init" + } + } + ] + } + } + */ + var root1 = document1.getRoot().rootTree().rootTreeNode as ElementNode + assertEquals( + mapOf("id" to "1", "value" to "init"), + (root1.children.first() as ElementNode).attributes, + ) + assertEquals( + mapOf("id" to "2", "value" to "init"), + (root1.children[1] as ElementNode).attributes, + ) + + var root2 = document2.getRoot().rootTree().rootTreeNode as ElementNode + assertEquals( + mapOf("id" to "1", "value" to "init"), + (root2.children.first() as ElementNode).attributes, + ) + assertEquals( + mapOf("id" to "2", "value" to "init"), + (root2.children[1] as ElementNode).attributes, + ) + + updateAndSync( + // client1 changes attributes on path [0] + Updater(client1, document1) { root, _ -> + root.rootTree().styleByPath(listOf(0), mapOf("value" to "changed")) + }, + // client2 deletes path[0] + Updater(client2, document2) { root, _ -> + root.rootTree().editByPath(listOf(0), listOf(1)) + }, + ) + + /* assert both documents are synced right + { + "t": { + "type": "root", + "children": [ + { + "type": "t", + "children": [], + "attributes": { + "id": "2", + "value": "init" + } + } + ] + } + } + */ + root1 = document1.getRoot().rootTree().rootTreeNode as ElementNode + assertEquals(1, root1.children.size) + assertEquals( + mapOf("id" to "2", "value" to "init"), + (root1.children.first() as ElementNode).attributes, + ) + + root2 = document2.getRoot().rootTree().rootTreeNode as ElementNode + assertEquals(1, root2.children.size) + assertEquals( + mapOf("id" to "2", "value" to "init"), + (root2.children.first() as ElementNode).attributes, + ) + + delay(500) + collectJobs.forEach(Job::cancel) + + // assert list of OperationInfo were emitted right + assertEquals( + listOf( + // client2 deleted on path [0] + TreeEditOpInfo( + 0, + 2, + listOf(0), + listOf(1), + null, + 0, + "$.t", + ), + ), + document1Ops, + ) + + assertEquals( + listOf( + // client1 set new tree + SetOpInfo("t", "$"), + // client1 initialized tree + TreeEditOpInfo( + 0, + 0, + listOf(0), + listOf(0), + listOf( + ElementNode("t", mapOf("id" to "1", "value" to "init")), + ElementNode("t", mapOf("id" to "2", "value" to "init")), + ), + 0, + "$.t", + ), + // client1 changed attributes on path [0] + /* assert style changes on already deleted path is not applied + { + "t": { + "type": "root", + "children": [ + { + "type": "t", + "children": [], + "attributes": { + "id": "2", + "value": "init" + } + } + ] + } + } + */ + ), + document2Ops, + ) + } + } + companion object { fun JsonObject.rootTree() = getAs("t") @@ -1629,7 +1818,7 @@ class JsonTreeTest { data class SimpleTreeEditOpInfo( val from: Int, val to: Int, - val nodes: JsonTree.TreeNode? = null, + val nodes: TreeNode? = null, ) } } diff --git a/yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt b/yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt index 366cc266e..fbc400ed2 100644 --- 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 { 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 {