From b5242dad062580394350bc5b21d8222bd844e5c6 Mon Sep 17 00:00:00 2001 From: jee-hyun-kim Date: Wed, 5 Jun 2024 11:45:50 +0900 Subject: [PATCH] Fix incorrect tree snapshot encoding/decoding --- .../kotlin/dev/yorkie/api/ElementConverter.kt | 5 ++-- .../dev/yorkie/document/crdt/CrdtTree.kt | 19 +++++------- .../main/kotlin/dev/yorkie/util/IndexTree.kt | 30 +++++++++++++++---- .../dev/yorkie/util/IndexTreeExtensions.kt | 11 +++++++ .../kotlin/dev/yorkie/api/ConverterTest.kt | 30 +++++++++++++++++++ 5 files changed, 75 insertions(+), 20 deletions(-) diff --git a/yorkie/src/main/kotlin/dev/yorkie/api/ElementConverter.kt b/yorkie/src/main/kotlin/dev/yorkie/api/ElementConverter.kt index ba1844875..5ae315e44 100644 --- a/yorkie/src/main/kotlin/dev/yorkie/api/ElementConverter.kt +++ b/yorkie/src/main/kotlin/dev/yorkie/api/ElementConverter.kt @@ -48,7 +48,7 @@ import dev.yorkie.document.crdt.TextValue import dev.yorkie.document.time.ActorID import dev.yorkie.document.time.TimeTicket.Companion.InitialTimeTicket import dev.yorkie.util.IndexTreeNode -import dev.yorkie.util.traverse +import dev.yorkie.util.traverseAll internal typealias PBJsonElement = dev.yorkie.api.v1.JSONElement internal typealias PBJsonElementSimple = dev.yorkie.api.v1.JSONElementSimple @@ -224,6 +224,7 @@ internal fun List.toCrdtTreeRootNode(): CrdtTreeNode? { }.takeIf { it > -1 }?.plus(i + 1) ?: continue nodes[parentIndex].prepend(nodes[i]) } + root.updateDescendantSize() return CrdtTree(root, InitialTimeTicket).root } @@ -315,7 +316,7 @@ internal fun RgaTreeList.toPBRgaNodes(): List { internal fun CrdtTreeNode.toPBTreeNodes(): List { val treeNode = this return buildList { - traverse(treeNode) { node, nodeDepth -> + traverseAll(treeNode) { node, nodeDepth -> val pbTreeNode = treeNode { id = node.id.toPBTreeNodeID() type = node.type 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 f303c8e5b..151baed75 100644 --- a/yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt +++ b/yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt @@ -1,5 +1,6 @@ package dev.yorkie.document.crdt +import androidx.annotation.VisibleForTesting import dev.yorkie.document.CrdtTreeNodeIDStruct import dev.yorkie.document.CrdtTreePosStruct import dev.yorkie.document.JsonSerializable @@ -15,6 +16,7 @@ import dev.yorkie.util.IndexTreeNodeList import dev.yorkie.util.TokenType import dev.yorkie.util.TreePos import dev.yorkie.util.TreeToken +import dev.yorkie.util.traverseAll import java.util.TreeMap public typealias TreePosRange = Pair @@ -46,7 +48,7 @@ internal data class CrdtTree( get() = indexTree.root.toTreeNode() init { - indexTree.traverse { node, _ -> + indexTree.traverseAll { node, _ -> nodeMapByID[node.id] = node } } @@ -54,6 +56,10 @@ internal data class CrdtTree( val size: Int get() = indexTree.size + @VisibleForTesting + val nodeSize: Int + get() = nodeMapByID.size + /** * Applies the given [attributes] of the given [range]. */ @@ -444,17 +450,6 @@ internal data class CrdtTree( return TreeOperationResult(changes, gcPairs) } - private fun traverseAll( - node: CrdtTreeNode, - depth: Int = 0, - action: ((CrdtTreeNode, Int) -> Unit), - ) { - node.allChildren.forEach { child -> - traverseAll(child, depth + 1, action) - } - action.invoke(node, depth) - } - private fun traverseInPosRange( fromParent: CrdtTreeNode, fromLeft: CrdtTreeNode, diff --git a/yorkie/src/main/kotlin/dev/yorkie/util/IndexTree.kt b/yorkie/src/main/kotlin/dev/yorkie/util/IndexTree.kt index 46767c1d5..176e691b0 100644 --- a/yorkie/src/main/kotlin/dev/yorkie/util/IndexTree.kt +++ b/yorkie/src/main/kotlin/dev/yorkie/util/IndexTree.kt @@ -138,6 +138,13 @@ internal class IndexTree>(val root: T) { traverse(root, 0, action) } + /** + * Traverses the whole tree (include tombstones) with postorder traversal. + */ + fun traverseAll(action: ((T, Int) -> Unit)) { + traverseAll(root, 0, action) + } + /** * Finds the position of the given [index] in the tree. */ @@ -370,7 +377,7 @@ internal data class TreePos>( * document of text-based editors. */ @Suppress("UNCHECKED_CAST") -internal abstract class IndexTreeNode>() { +internal abstract class IndexTreeNode> { abstract val type: String protected abstract val childNodes: IndexTreeNodeList @@ -422,7 +429,8 @@ internal abstract class IndexTreeNode>() { var onRemovedListener: OnRemovedListener? = null /** - * Updates the size of the ancestors. + * Updates the size of the ancestors. It is used when + * the size of the node is changed. */ fun updateAncestorSize() { var parent = parent @@ -434,6 +442,20 @@ internal abstract class IndexTreeNode>() { } } + /** + * Updates the size of the descendants. It is used when + * the tree is newly created and the size of the descendants is not calculated. + */ + fun updateDescendantSize(): Int { + if (isRemoved) { + size = 0 + return 0 + } + + size += children.sumOf { it.updateDescendantSize() } + return paddedSize + } + fun findOffset(node: T): Int { check(!isText) { "Text node cannot have children" @@ -474,10 +496,6 @@ internal abstract class IndexTreeNode>() { childNodes.add(0, node) node.parent = this as T - - if (!node.isRemoved) { - node.updateAncestorSize() - } } /** diff --git a/yorkie/src/main/kotlin/dev/yorkie/util/IndexTreeExtensions.kt b/yorkie/src/main/kotlin/dev/yorkie/util/IndexTreeExtensions.kt index a0c9a4b19..f908bde5e 100644 --- a/yorkie/src/main/kotlin/dev/yorkie/util/IndexTreeExtensions.kt +++ b/yorkie/src/main/kotlin/dev/yorkie/util/IndexTreeExtensions.kt @@ -11,6 +11,17 @@ internal fun > traverse( action.invoke(node, depth) } +internal fun > traverseAll( + node: T, + depth: Int = 0, + action: ((T, Int) -> Unit), +) { + node.allChildren.forEach { child -> + traverseAll(child, depth + 1, action) + } + action.invoke(node, depth) +} + internal fun > findCommonAncestor(node1: T, node2: T): T? { if (node1 == node2) { return node1 diff --git a/yorkie/src/test/kotlin/dev/yorkie/api/ConverterTest.kt b/yorkie/src/test/kotlin/dev/yorkie/api/ConverterTest.kt index 643ee1970..b5df75b5d 100644 --- a/yorkie/src/test/kotlin/dev/yorkie/api/ConverterTest.kt +++ b/yorkie/src/test/kotlin/dev/yorkie/api/ConverterTest.kt @@ -4,6 +4,7 @@ import com.google.protobuf.kotlin.toByteStringUtf8 import dev.yorkie.api.v1.jSONElement import dev.yorkie.api.v1.operation import dev.yorkie.core.PresenceChange +import dev.yorkie.document.Document import dev.yorkie.document.change.Change import dev.yorkie.document.change.ChangeID import dev.yorkie.document.change.ChangePack @@ -25,6 +26,9 @@ import dev.yorkie.document.crdt.RgaTreeList import dev.yorkie.document.crdt.RgaTreeSplit import dev.yorkie.document.crdt.RgaTreeSplitNodeID import dev.yorkie.document.crdt.RgaTreeSplitPos +import dev.yorkie.document.json.JsonObject +import dev.yorkie.document.json.JsonTree +import dev.yorkie.document.json.TreeBuilder.element import dev.yorkie.document.operation.AddOperation import dev.yorkie.document.operation.EditOperation import dev.yorkie.document.operation.IncreaseOperation @@ -44,6 +48,7 @@ import dev.yorkie.document.time.TimeTicket.Companion.MaxTimeTicket import dev.yorkie.util.IndexTreeNode.Companion.DEFAULT_ROOT_TYPE import java.util.Date import kotlin.test.assertEquals +import kotlinx.coroutines.test.runTest import org.junit.Assert.assertThrows import org.junit.Test @@ -413,6 +418,31 @@ class ConverterTest { } } + @Test + fun `should encode and decode tree properly`() = runTest { + val document = Document(Document.Key("")) + document.updateAsync { root, _ -> + root.setNewTree( + "t", + element("r") { + element("p") { text { "12" } } + element("p") { text { "34" } } + }, + ).editByPath(listOf(0, 1), listOf(1, 1)) + }.await() + + fun JsonObject.tree() = getAs("t") + + assertEquals("

14

", document.getRoot().tree().toXml()) + assertEquals(4, document.getRoot().tree().size) + + val tree = document.getRoot().target["t"] + val bytes = tree.toByteString() + val obj = bytes.toCrdtTree() + assertEquals(document.getRoot().tree().target.nodeSize, obj.nodeSize) + assertEquals(document.getRoot().tree().size, obj.size) + } + private class TestOperation( override val parentCreatedAt: TimeTicket, override var executedAt: TimeTicket,