Skip to content

Commit

Permalink
Fix invalid TreeChanges in concurrent Tree.Style (#149)
Browse files Browse the repository at this point in the history
  • Loading branch information
@skhugh
skhugh authored Jan 25, 2024
1 parent 0c12299 commit 8661963
Show file tree
Hide file tree
Showing 2 changed files with 212 additions and 24 deletions.
191 changes: 190 additions & 1 deletion yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<OperationInfo>()
val document2Ops = mutableListOf<OperationInfo>()

val collectJobs = listOf(
launch(start = CoroutineStart.UNDISPATCHED) {
document1.events.filterIsInstance<RemoteChange>()
.collect {
document1Ops.addAll(it.changeInfo.operations)
}
},
launch(start = CoroutineStart.UNDISPATCHED) {
document2.events.filterIsInstance<RemoteChange>()
.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<OperationInfo>(
// 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<JsonTree>("t")
Expand Down Expand Up @@ -1629,7 +1818,7 @@ class JsonTreeTest {
data class SimpleTreeEditOpInfo(
val from: Int,
val to: Int,
val nodes: JsonTree.TreeNode? = null,
val nodes: TreeNode? = null,
)
}
}
45 changes: 22 additions & 23 deletions yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down

0 comments on commit 8661963

Please sign in to comment.