Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect tree snapshot encoding/decoding #198

Merged
merged 1 commit into from
Jun 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions yorkie/src/main/kotlin/dev/yorkie/api/ElementConverter.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -224,6 +224,7 @@ internal fun List<PBTreeNode>.toCrdtTreeRootNode(): CrdtTreeNode? {
}.takeIf { it > -1 }?.plus(i + 1) ?: continue
nodes[parentIndex].prepend(nodes[i])
}
root.updateDescendantSize()
return CrdtTree(root, InitialTimeTicket).root
}

Expand Down Expand Up @@ -315,7 +316,7 @@ internal fun RgaTreeList.toPBRgaNodes(): List<PBRgaNode> {
internal fun CrdtTreeNode.toPBTreeNodes(): List<PBTreeNode> {
val treeNode = this
return buildList {
traverse(treeNode) { node, nodeDepth ->
traverseAll(treeNode) { node, nodeDepth ->
val pbTreeNode = treeNode {
id = node.id.toPBTreeNodeID()
type = node.type
Expand Down
19 changes: 7 additions & 12 deletions yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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<CrdtTreePos, CrdtTreePos>
Expand Down Expand Up @@ -46,14 +48,18 @@ internal data class CrdtTree(
get() = indexTree.root.toTreeNode()

init {
indexTree.traverse { node, _ ->
indexTree.traverseAll { node, _ ->
nodeMapByID[node.id] = node
}
}

val size: Int
get() = indexTree.size

@VisibleForTesting
val nodeSize: Int
get() = nodeMapByID.size

/**
* Applies the given [attributes] of the given [range].
*/
Expand Down Expand Up @@ -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,
Expand Down
30 changes: 24 additions & 6 deletions yorkie/src/main/kotlin/dev/yorkie/util/IndexTree.kt
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,13 @@ internal class IndexTree<T : IndexTreeNode<T>>(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.
*/
Expand Down Expand Up @@ -370,7 +377,7 @@ internal data class TreePos<T : IndexTreeNode<T>>(
* document of text-based editors.
*/
@Suppress("UNCHECKED_CAST")
internal abstract class IndexTreeNode<T : IndexTreeNode<T>>() {
internal abstract class IndexTreeNode<T : IndexTreeNode<T>> {
abstract val type: String

protected abstract val childNodes: IndexTreeNodeList<T>
Expand Down Expand Up @@ -422,7 +429,8 @@ internal abstract class IndexTreeNode<T : IndexTreeNode<T>>() {
var onRemovedListener: OnRemovedListener<T>? = 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
Expand All @@ -434,6 +442,20 @@ internal abstract class IndexTreeNode<T : IndexTreeNode<T>>() {
}
}

/**
* 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"
Expand Down Expand Up @@ -474,10 +496,6 @@ internal abstract class IndexTreeNode<T : IndexTreeNode<T>>() {

childNodes.add(0, node)
node.parent = this as T

if (!node.isRemoved) {
node.updateAncestorSize()
}
}

/**
Expand Down
11 changes: 11 additions & 0 deletions yorkie/src/main/kotlin/dev/yorkie/util/IndexTreeExtensions.kt
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,17 @@ internal fun <T : IndexTreeNode<T>> traverse(
action.invoke(node, depth)
}

internal fun <T : IndexTreeNode<T>> 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 <T : IndexTreeNode<T>> findCommonAncestor(node1: T, node2: T): T? {
if (node1 == node2) {
return node1
Expand Down
30 changes: 30 additions & 0 deletions yorkie/src/test/kotlin/dev/yorkie/api/ConverterTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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

Expand Down Expand Up @@ -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<JsonTree>("t")

assertEquals("<r><p>14</p></r>", 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,
Expand Down
Loading