Handle concurrent editing and styling in Tree (#178)

* Handle concurrent editing and styling in Tree

.github/workflows/ci.yml

@@ -116,10 +116,9 @@ jobs:
       - uses: actions/download-artifact@v2
       - uses: codecov/codecov-action@v3
         with:
+          token: ${{ secrets.CODECOV_TOKEN }}
           fail_ci_if_error: true
           verbose: false
-        env:
-          CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
 
   microbenchmarks:
     name: Microbenchmarks

docker/docker-compose-ci.yml

@@ -2,7 +2,7 @@ version: '3.3'
 
 services:
   yorkie:
-    image: 'yorkieteam/yorkie:0.4.15'
+    image: 'yorkieteam/yorkie:0.4.19'
     container_name: 'yorkie'
     command: ['server', '--mongo-connection-uri', 'mongodb://mongo:27017']
     restart: always

docker/docker-compose.yml

@@ -2,7 +2,7 @@ version: '3.3'
 
 services:
   yorkie:
-    image: 'yorkieteam/yorkie:0.4.15'
+    image: 'yorkieteam/yorkie:0.4.19'
     container_name: 'yorkie'
     command: ['server', '--enable-pprof']
     restart: always

gradle.properties

@@ -11,7 +11,7 @@ android.suppressUnsupportedOptionWarnings=android.suppressUnsupportedOptionWarni
 kotlin.code.style=official
 kotlin.mpp.stability.nowarn=true
 GROUP=dev.yorkie
-VERSION_NAME=0.4.16
+VERSION_NAME=0.4.19
 POM_DESCRIPTION=Document store for building collaborative editing applications.
 POM_INCEPTION_YEAR=2022
 POM_URL=https://github.com/yorkie-team/yorkie-android-sdk

yorkie/proto/yorkie/v1/resources.proto

@@ -134,6 +134,7 @@ message Operation {
     map<string, string> attributes = 4;
     TimeTicket executed_at = 5;
     repeated string attributes_to_remove = 6;
+    map<string, TimeTicket> created_at_map_by_actor = 7;
   }
 
   oneof body {

yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt

@@ -20,9 +20,11 @@ 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 dev.yorkie.gson
 import kotlin.test.assertEquals
 import kotlin.test.assertIs
+import kotlin.test.assertTrue
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.CoroutineStart
 import kotlinx.coroutines.ExperimentalCoroutinesApi
@@ -2099,6 +2101,66 @@ class JsonTreeTest {
         }
     }
 
+    @Test
+    fun test_tree_style_events_concurrency() {
+        withTwoClientsAndDocuments(syncMode = Manual) { c1, c2, d1, d2, _ ->
+            updateAndSync(
+                Updater(c1, d1) { root, _ ->
+                    root.setNewTree(
+                        "t",
+                        element("doc") {
+                            element("p") {
+                                text { "hello" }
+                                attr { "italic" to true }
+                            }
+                        },
+                    )
+                },
+                Updater(c2, d2),
+            )
+            assertTreesXmlEquals("""<doc><p italic="true">hello</p></doc>""", d1, d2)
+
+            val d1Events = mutableListOf<TreeStyleOpInfo>()
+            val d2Events = mutableListOf<TreeStyleOpInfo>()
+
+            val collectJob = launch(start = CoroutineStart.UNDISPATCHED) {
+                launch(start = CoroutineStart.UNDISPATCHED) {
+                    d1.events.filterIsInstance<RemoteChange>()
+                        .map {
+                            it.changeInfo.operations.filterIsInstance<TreeStyleOpInfo>()
+                        }
+                        .collect(d1Events::addAll)
+                }
+                launch(start = CoroutineStart.UNDISPATCHED) {
+                    d2.events.filterIsInstance<RemoteChange>()
+                        .map {
+                            it.changeInfo.operations.filterIsInstance<TreeStyleOpInfo>()
+                        }
+                        .collect(d2Events::addAll)
+                }
+            }
+
+            d1.updateAsync { root, _ ->
+                root.rootTree().style(0, 1, mapOf("bold" to "true"))
+            }.await()
+
+            d2.updateAsync { root, _ ->
+                root.rootTree().style(0, 1, mapOf("bold" to "false"))
+            }.await()
+
+            c1.syncAsync().await()
+            c2.syncAsync().await()
+            c1.syncAsync().await()
+
+            assertEquals(d1.getRoot().rootTree().toXml(), d2.getRoot().rootTree().toXml())
+
+            assertEquals("false", d1Events.single().attributes["bold"])
+            assertTrue(d2Events.isEmpty())
+
+            collectJob.cancel()
+        }
+    }
+
     companion object {
 
         fun JsonObject.rootTree() = getAs<JsonTree>("t")

yorkie/src/main/kotlin/dev/yorkie/api/OperationConverter.kt

@@ -109,6 +109,8 @@ internal fun List<PBOperation>.toOperations(): List<Operation> {
                 attributes = it.treeStyle.attributesMap,
                 executedAt = it.treeStyle.executedAt.toTimeTicket(),
                 attributesToRemove = it.treeStyle.attributesToRemoveList,
+                maxCreatedAtMapByActor = it.treeStyle.createdAtMapByActorMap.entries
+                    .associate { (key, value) -> ActorID(key) to value.toTimeTicket() },
             )
 
             else -> throw IllegalArgumentException("unimplemented operation")
@@ -231,6 +233,9 @@ internal fun Operation.toPBOperation(): PBOperation {
                         attributes[key] = value
                     }
                     operation.attributesToRemove?.forEach { attributesToRemove.add(it) }
+                    operation.maxCreatedAtMapByActor?.forEach {
+                        createdAtMapByActor[it.key.value] = it.value.toPBTimeTicket()
+                    }
                 }
             }
         }

yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtText.kt

@@ -33,7 +33,7 @@ internal data class CrdtText(
         value: String,
         executedAt: TimeTicket,
         attributes: Map<String, String>? = null,
-        latestCreatedAtMapByActor: Map<ActorID, TimeTicket>? = null,
+        maxCreatedAtMapByActor: Map<ActorID, TimeTicket>? = null,
     ): TextOperationResult {
         val textValue = if (value.isNotEmpty()) {
             TextValue(value).apply {
@@ -43,11 +43,11 @@ internal data class CrdtText(
             null
         }
 
-        val (caretPos, latestCreatedAtMap, contentChanges) = rgaTreeSplit.edit(
+        val (caretPos, maxCreatedAtMap, contentChanges) = rgaTreeSplit.edit(
             range,
             executedAt,
             textValue,
-            latestCreatedAtMapByActor,
+            maxCreatedAtMapByActor,
         )
 
         val changes = contentChanges.map {
@@ -63,7 +63,7 @@ internal data class CrdtText(
         if (value.isNotEmpty() && attributes != null) {
             changes[changes.lastIndex] = changes.last().copy(attributes = attributes)
         }
-        return TextOperationResult(latestCreatedAtMap, changes, caretPos to caretPos)
+        return TextOperationResult(maxCreatedAtMap, changes, caretPos to caretPos)
     }
 
     /**
@@ -75,7 +75,7 @@ internal data class CrdtText(
         range: RgaTreeSplitPosRange,
         attributes: Map<String, String>,
         executedAt: TimeTicket,
-        latestCreatedAtMapByActor: Map<ActorID, TimeTicket>? = null,
+        maxCreatedAtMapByActor: Map<ActorID, TimeTicket>? = null,
     ): TextOperationResult {
         // 1. Split nodes with from and to.
         val toRight = rgaTreeSplit.findNodeWithSplit(range.second, executedAt).second
@@ -86,18 +86,18 @@ internal data class CrdtText(
         val createdAtMapByActor = mutableMapOf<ActorID, TimeTicket>()
         val toBeStyleds = nodes.mapNotNull { node ->
             val actorID = node.createdAt.actorID
-            val latestCreatedAt = if (latestCreatedAtMapByActor?.isNotEmpty() == true) {
-                latestCreatedAtMapByActor[actorID] ?: TimeTicket.InitialTimeTicket
+            val maxCreatedAt = if (maxCreatedAtMapByActor?.isNotEmpty() == true) {
+                maxCreatedAtMapByActor[actorID] ?: TimeTicket.InitialTimeTicket
             } else {
                 TimeTicket.MaxTimeTicket
             }
 
             node.takeIf {
-                it.canStyle(executedAt, latestCreatedAt)
+                it.canStyle(executedAt, maxCreatedAt)
             }?.also {
-                val updatedLatestCreatedAt = createdAtMapByActor[actorID]
+                val updatedMaxCreatedAt = createdAtMapByActor[actorID]
                 val updatedCreatedAt = node.createdAt
-                if (updatedLatestCreatedAt == null || updatedLatestCreatedAt < updatedCreatedAt) {
+                if (updatedMaxCreatedAt == null || updatedMaxCreatedAt < updatedCreatedAt) {
                     createdAtMapByActor[actorID] = updatedCreatedAt
                 }
             }

yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt

@@ -56,34 +56,46 @@ internal data class CrdtTree(
         range: TreePosRange,
         attributes: Map<String, String>?,
         executedAt: TimeTicket,
-    ): List<TreeChange> {
+        maxCreatedAtMapByActor: Map<ActorID, TimeTicket>? = null,
+    ): Pair<List<TreeChange>, Map<ActorID, TimeTicket>> {
         val (fromParent, fromLeft) = findNodesAndSplitText(range.first, executedAt)
         val (toParent, toLeft) = findNodesAndSplitText(range.second, 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,
-                        ),
-                    )
+        val changes = mutableListOf<TreeChange>()
+        val createdAtMapByActor = mutableMapOf<ActorID, TimeTicket>()
+
+        traverseInPosRange(
+            fromParent = fromParent,
+            fromLeft = fromLeft,
+            toParent = toParent,
+            toLeft = toLeft,
+        ) { (node, _), _ ->
+            val actorID = node.createdAt.actorID
+            val maxCreatedAt = maxCreatedAtMapByActor?.let {
+                maxCreatedAtMapByActor[actorID] ?: InitialTimeTicket
+            } ?: MaxTimeTicket
+
+            if (node.canStyle(executedAt, maxCreatedAt) && attributes != null) {
+                val max = createdAtMapByActor[actorID]
+                val createdAt = node.createdAt
+                if (max == null || max < createdAt) {
+                    createdAtMapByActor[actorID] = createdAt
+                }
+
+                val affectedKeys = node.setAttributes(attributes, executedAt)
+                if (affectedKeys.isNotEmpty()) {
+                    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.filterKeys { it in affectedKeys },
+                    ).let(changes::add)
                 }
             }
         }
+        return changes to createdAtMapByActor
     }
 
     private fun toPath(parentNode: CrdtTreeNode, leftSiblingNode: CrdtTreeNode): List<Int> {
@@ -133,7 +145,7 @@ internal data class CrdtTree(
         splitLevel: Int,
         executedAt: TimeTicket,
         issueTimeTicket: (() -> TimeTicket)? = null,
-        latestCreatedAtMapByActor: Map<ActorID, TimeTicket>? = null,
+        maxCreatedAtMapByActor: Map<ActorID, TimeTicket>? = null,
     ): Pair<List<TreeChange>, Map<ActorID, TimeTicket>> {
         // 01. find nodes from the given range and split nodes.
         val (fromParent, fromLeft) = findNodesAndSplitText(range.first, executedAt)
@@ -145,7 +157,7 @@ internal data class CrdtTree(
         val nodesToBeRemoved = mutableListOf<CrdtTreeNode>()
         val tokensToBeRemoved = mutableListOf<CrdtTreeToken>()
         val toBeMovedToFromParents = mutableListOf<CrdtTreeNode>()
-        val latestCreatedAtMap = mutableMapOf<ActorID, TimeTicket>()
+        val maxCreatedAtMap = mutableMapOf<ActorID, TimeTicket>()
 
         traverseInPosRange(
             fromParent = fromParent,
@@ -160,16 +172,16 @@ internal data class CrdtTree(
             }
 
             val actorID = node.createdAt.actorID
-            val latestCreatedAt = latestCreatedAtMapByActor?.let {
-                latestCreatedAtMapByActor[actorID] ?: InitialTimeTicket
+            val maxCreatedAt = maxCreatedAtMapByActor?.let {
+                maxCreatedAtMapByActor[actorID] ?: InitialTimeTicket
             } ?: MaxTimeTicket
 
-            if (node.canDelete(executedAt, latestCreatedAt) || node.parent in nodesToBeRemoved) {
-                val latest = latestCreatedAtMap[actorID]
+            if (node.canDelete(executedAt, maxCreatedAt) || node.parent in nodesToBeRemoved) {
+                val max = maxCreatedAtMap[actorID]
                 val createdAt = node.createdAt
 
-                if (latest == null || latest < createdAt) {
-                    latestCreatedAtMap[actorID] = createdAt
+                if (max == null || max < createdAt) {
+                    maxCreatedAtMap[actorID] = createdAt
                 }
 
                 if (tokenType == TokenType.Text || tokenType == TokenType.Start) {
@@ -265,7 +277,7 @@ internal data class CrdtTree(
                 }
             }
         }
-        return changes to latestCreatedAtMap
+        return changes to maxCreatedAtMap
     }
 
     /**
@@ -806,12 +818,10 @@ internal data class CrdtTreeNode private constructor(
         return split
     }
 
-    fun setAttribute(
-        key: String,
-        value: String,
-        executedAt: TimeTicket,
-    ) {
-        _attributes.set(key, value, executedAt)
+    fun setAttributes(attributes: Map<String, String>, executedAt: TimeTicket): Set<String> {
+        return attributes.filter { (key, value) ->
+            _attributes.set(key, value, executedAt)
+        }.keys.toSet()
     }
 
     fun removeAttribute(key: String, executedAt: TimeTicket) {
@@ -865,8 +875,15 @@ internal data class CrdtTreeNode private constructor(
     /**
      * Checks if node is able to delete.
      */
-    fun canDelete(executedAt: TimeTicket, latestCreatedAt: TimeTicket): Boolean {
-        return createdAt <= latestCreatedAt && (removedAt == null || removedAt < executedAt)
+    fun canDelete(executedAt: TimeTicket, maxCreatedAt: TimeTicket): Boolean {
+        return createdAt <= maxCreatedAt && (removedAt == null || removedAt < executedAt)
+    }
+
+    fun canStyle(executedAt: TimeTicket, maxCreatedAt: TimeTicket): Boolean {
+        if (isText) {
+            return false
+        }
+        return createdAt <= maxCreatedAt && (removedAt == null || removedAt < executedAt)
     }
 
     @Suppress("FunctionName")