Fix invalid tree style changes (#167)

* Fix invalid tree style changes * fix lint error
yorkie-team · May 17, 2024 · 6f76389 · 6f76389
1 parent 4b063d8
commit 6f76389
Show file tree

Hide file tree

Showing 3 changed files with 80 additions and 21 deletions.
diff --git a/Sources/Document/CRDT/CRDTTree.swift b/Sources/Document/CRDT/CRDTTree.swift
@@ -588,7 +588,6 @@ class CRDTTree: CRDTGCElement {
         let (toParent, toLeft) = try self.findNodesAndSplitText(range.1, editedAt)
 
         var changes: [TreeChange] = []
-        let value = attributes != nil ? TreeChangeValue.attributes(attributes!) : nil
         var createdAtMapByActor = [String: TimeTicket]()
 
         try self.traverseInPosRange(fromParent, fromLeft, toParent, toLeft) { token, _ in
@@ -606,19 +605,30 @@ class CRDTTree: CRDTGCElement {
                 if node.attrs == nil {
                     node.attrs = RHT()
                 }
-                for (key, value) in attributes ?? [:] {
-                    node.attrs?.set(key: key, value: value, executedAt: editedAt)
+                var affectedKeys = Set<String>()
+                for (key, value) in attributes ?? [:] where node.attrs?.set(key: key, value: value, executedAt: editedAt) ?? false {
+                    affectedKeys.insert(key)
                 }
 
-                try changes.append(TreeChange(actor: editedAt.actorID,
-                                              type: .style,
-                                              from: self.toIndex(fromParent, fromLeft),
-                                              to: self.toIndex(toParent, toLeft),
-                                              fromPath: self.toPath(fromParent, fromLeft),
-                                              toPath: self.toPath(toParent, toLeft),
-                                              value: value,
-                                              splitLevel: 0) // dummy value.
-                )
+                if !affectedKeys.isEmpty {
+                    var affectedAttrs = [String: String]()
+
+                    for affectedKey in affectedKeys {
+                        if let attr = attributes?[affectedKey] {
+                            affectedAttrs[affectedKey] = attr
+                        }
+                    }
+
+                    try changes.append(TreeChange(actor: editedAt.actorID,
+                                                  type: .style,
+                                                  from: self.toIndex(fromParent, fromLeft),
+                                                  to: self.toIndex(toParent, toLeft),
+                                                  fromPath: self.toPath(fromParent, fromLeft),
+                                                  toPath: self.toPath(toParent, toLeft),
+                                                  value: TreeChangeValue.attributes(affectedAttrs),
+                                                  splitLevel: 0) // dummy value.
+                    )
+                }
             }
         }
 

diff --git a/Sources/Document/CRDT/RHT.swift b/Sources/Document/CRDT/RHT.swift
@@ -37,19 +37,22 @@ class RHT {
     /**
      * `set` sets the value of the given key.
      */
-    func set(key: String, value: String, executedAt: TimeTicket) {
-        if let prev = nodeMapByKey[key] {
-            if executedAt.after(prev.updatedAt) {
-                if !prev.isRemoved {
-                    self.numberOfRemovedElement -= 1
-                }
-                let node = RHTNode(key: key, value: value, updatedAt: executedAt, isRemoved: false)
-                self.nodeMapByKey[key] = node
+    @discardableResult
+    func set(key: String, value: String, executedAt: TimeTicket) -> Bool {
+        let prev = self.nodeMapByKey[key]
+
+        if prev == nil || executedAt.after(prev!.updatedAt) {
+            if prev != nil && !prev!.isRemoved {
+                self.numberOfRemovedElement -= 1
             }
-        } else {
+
             let node = RHTNode(key: key, value: value, updatedAt: executedAt, isRemoved: false)
             self.nodeMapByKey[key] = node
+
+            return true
         }
+
+        return false
     }
 
     /**

diff --git a/Tests/Integration/TreeIntegrationTests.swift b/Tests/Integration/TreeIntegrationTests.swift
@@ -3765,6 +3765,52 @@ final class TreeIntegrationTreeChangeGeneration: XCTestCase {
         }
     }
 
+    func test_concurrent_style_and_style() async throws {
+        try await withTwoClientsAndDocuments(self.description) { c1, d1, c2, d2 in
+            try await d1.update { root, _ in
+                root.t = JSONTree(initialRoot:
+                    JSONTreeElementNode(type: "doc",
+                                        children: [
+                                            JSONTreeElementNode(type: "p",
+                                                                children: [
+                                                                    JSONTreeTextNode(value: "hello")
+                                                                ])
+                                        ])
+                )
+            }
+
+            try await c1.sync()
+            try await c2.sync()
+
+            var d1XML = await(d1.getRoot().t as? JSONTree)?.toXML()
+            var d2XML = await(d2.getRoot().t as? JSONTree)?.toXML()
+            XCTAssertEqual(d1XML, d2XML)
+            XCTAssertEqual(d1XML, /* html */ "<doc><p>hello</p></doc>")
+
+            await subscribeDocs(d1,
+                                d2,
+                                [TreeStyleOpInfoForDebug(from: 0, to: 1, value: ["bold": "true"], fromPath: nil),
+                                 TreeStyleOpInfoForDebug(from: 0, to: 1, value: ["bold": "false"], fromPath: nil)],
+                                [TreeStyleOpInfoForDebug(from: 0, to: 1, value: ["bold": "false"], fromPath: nil)])
+
+            try await d1.update { root, _ in
+                try (root.t as? JSONTree)?.style(0, 1, ["bold": "true"])
+            }
+            try await d2.update { root, _ in
+                try (root.t as? JSONTree)?.style(0, 1, ["bold": "false"])
+            }
+
+            try await c1.sync()
+            try await c2.sync()
+            try await c1.sync()
+
+            d1XML = await(d1.getRoot().t as? JSONTree)?.toXML()
+            d2XML = await(d2.getRoot().t as? JSONTree)?.toXML()
+            XCTAssertEqual(d1XML, d2XML)
+            XCTAssertEqual(d1XML, /* html */ "<doc><p bold=\"false\">hello</p></doc>")
+        }
+    }
+
     func test_emoji() async throws {
         try await withTwoClientsAndDocuments(self.description) { c1, d1, c2, d2 in
             try await d1.update { root, _ in