From be724652bd67abda11590be537407d6842c80858 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EC=95=88=EC=A0=95=EA=B7=A0?= Date: Wed, 12 Jun 2024 01:19:06 +0900 Subject: [PATCH 1/6] Fix incorrect indexes in TreeChange --- Sources/API/Converter.swift | 4 +- Sources/Document/CRDT/CRDTTree.swift | 66 +++++++---- Sources/Document/CRDT/RHT.swift | 2 +- Sources/Document/Json/JSONTree.swift | 6 +- Sources/Document/Operation/Operation.swift | 23 +++- .../Operation/TreeEditOperation.swift | 4 +- .../Operation/TreeSytleOperation.swift | 18 +-- Sources/Util/IndexTree.swift | 15 +++ Tests/Integration/IntegrationHelper.swift | 13 +-- Tests/Integration/TreeConcurrencyTests.swift | 7 +- Tests/Integration/TreeIntegrationTests.swift | 106 +++++++++++++++++- 11 files changed, 211 insertions(+), 53 deletions(-) diff --git a/Sources/API/Converter.swift b/Sources/API/Converter.swift index 7a8c1fd0..000dd9c5 100644 --- a/Sources/API/Converter.swift +++ b/Sources/API/Converter.swift @@ -1005,10 +1005,10 @@ extension Converter { static func fromTreeNode(_ pbTreeNode: PbTreeNode) -> CRDTTreeNode { let id = fromTreeNodeID(pbTreeNode.id) let node = CRDTTreeNode(id: id, type: pbTreeNode.type) - + if node.isText { node.value = pbTreeNode.value as NSString - } else { + } else if !pbTreeNode.attributes.isEmpty { node.attrs = RHT() pbTreeNode.attributes.forEach { key, value in diff --git a/Sources/Document/CRDT/CRDTTree.swift b/Sources/Document/CRDT/CRDTTree.swift index 6eae2c56..75d91b50 100644 --- a/Sources/Document/CRDT/CRDTTree.swift +++ b/Sources/Document/CRDT/CRDTTree.swift @@ -98,12 +98,12 @@ extension CRDTTreePos { } /** - * `toTreeNodes` converts the pos to parent and left sibling nodes. + * `toTreeNodePair` converts the pos to parent and left sibling nodes. * If the position points to the middle of a node, then the left sibling node * is the node that contains the position. Otherwise, the left sibling node is * the node that is located at the left of the position. */ - func toTreeNodes(tree: CRDTTree) throws -> (CRDTTreeNode, CRDTTreeNode) { + func toTreeNodePair(tree: CRDTTree) throws -> TreeNodePair { let parentID = self.parentID let leftSiblingID = self.leftSiblingID let parentNode = tree.findFloorNode(parentID) @@ -232,6 +232,12 @@ public struct CRDTTreeNodeIDStruct: Codable { */ typealias TreePosRange = (CRDTTreePos, CRDTTreePos) +/** + * `TreeNodePair` represents a pair of CRDTTreeNode. It represents the position + * of the node in the tree with the left and parent nodes. + */ +typealias TreeNodePair = (CRDTTreeNode, CRDTTreeNode) + /** * `TreePosStructRange` represents a pair of CRDTTreePosStruct. */ @@ -597,9 +603,9 @@ class CRDTTree: CRDTElement { * This is different from `TreePos` which is a position of the tree in the * physical perspective. */ - func findNodesAndSplitText(_ pos: CRDTTreePos, _ editedAt: TimeTicket? = nil) throws -> (CRDTTreeNode, CRDTTreeNode) { + func findNodesAndSplitText(_ pos: CRDTTreePos, _ editedAt: TimeTicket? = nil) throws -> TreeNodePair { // 01. Find the parent and left sibling node of the given position. - let (parent, leftSibling) = try pos.toTreeNodes(tree: self) + let (parent, leftSibling) = try pos.toTreeNodePair(tree: self) var leftNode = leftSibling // 02. Determine whether the position is left-most and the exact parent @@ -636,7 +642,7 @@ class CRDTTree: CRDTElement { * `style` applies the given attributes of the given range. */ @discardableResult - func style(_ range: TreePosRange, _ attributes: [String: String]?, _ editedAt: TimeTicket, _ maxCreatedAtMapByActor: [String: TimeTicket]?) throws -> ([String: TimeTicket], [GCPair], [TreeChange]) { + func style(_ range: TreePosRange, _ attributes: [String: String]?, _ editedAt: TimeTicket, _ maxCreatedAtMapByActor: [String: TimeTicket]? = nil) throws -> ([String: TimeTicket], [GCPair], [TreeChange]) { let (fromParent, fromLeft) = try self.findNodesAndSplitText(range.0, editedAt) let (toParent, toLeft) = try self.findNodesAndSplitText(range.1, editedAt) @@ -646,10 +652,10 @@ class CRDTTree: CRDTElement { try self.traverseInPosRange(fromParent, fromLeft, toParent, toLeft) { token, _ in let (node, _) = token let actorID = node.createdAt.actorID - var maxCreatedAt: TimeTicket? = maxCreatedAtMapByActor != nil ? maxCreatedAtMapByActor?[actorID] ?? TimeTicket.initial : TimeTicket.max + let maxCreatedAt = maxCreatedAtMapByActor != nil ? maxCreatedAtMapByActor![actorID] ?? TimeTicket.initial : TimeTicket.max - if node.canStyle(editedAt, maxCreatedAt!), !node.isText, let attributes { - maxCreatedAt = createdAtMapByActor[actorID] + if node.canStyle(editedAt, maxCreatedAt), !node.isText, let attributes { + let maxCreatedAt = createdAtMapByActor[actorID] let createdAt = node.createdAt if maxCreatedAt == nil || createdAt.after(maxCreatedAt!) { createdAtMapByActor[actorID] = createdAt @@ -666,13 +672,16 @@ class CRDTTree: CRDTElement { } } + let parentOfNode = node.parent! + let previousNode = node.prevSibling ?? node.parent! + if !affectedAttrs.isEmpty { 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), + from: self.toIndex(parentOfNode, previousNode), + to: self.toIndex(node, node), + fromPath: self.toPath(parentOfNode, previousNode), + toPath: self.toPath(node, node), value: TreeChangeValue.attributes(affectedAttrs), splitLevel: 0) // dummy value. ) @@ -690,16 +699,26 @@ class CRDTTree: CRDTElement { /** * `removeStyle` removes the given attributes of the given range. */ - func removeStyle(_ range: TreePosRange, _ attributesToRemove: [String], _ editedAt: TimeTicket) throws -> ([GCPair], [TreeChange]) { + func removeStyle(_ range: TreePosRange, _ attributesToRemove: [String], _ editedAt: TimeTicket, _ maxCreatedAtMapByActor: [String: TimeTicket]? = nil) throws -> ([String: TimeTicket], [GCPair], [TreeChange]) { let (fromParent, fromLeft) = try self.findNodesAndSplitText(range.0, editedAt) let (toParent, toLeft) = try self.findNodesAndSplitText(range.1, editedAt) var changes: [TreeChange] = [] + var createdAtMapByActor = [String: TimeTicket]() var pairs = [GCPair]() let value = TreeChangeValue.attributesToRemove(attributesToRemove) try self.traverseInPosRange(fromParent, fromLeft, toParent, toLeft) { token, _ in let (node, _) = token - if node.isRemoved == false, node.isText == false { + let actorID = node.createdAt.actorID + let maxCreatedAt = maxCreatedAtMapByActor != nil ? maxCreatedAtMapByActor![actorID] ?? TimeTicket.initial : TimeTicket.max + + if node.canStyle(editedAt, maxCreatedAt), !attributesToRemove.isEmpty { + let maxCreatedAt = createdAtMapByActor[actorID] + let createdAt = node.createdAt + if maxCreatedAt == nil || createdAt.after(maxCreatedAt!) { + createdAtMapByActor[actorID] = createdAt + } + if node.attrs == nil { node.attrs = RHT() } @@ -710,19 +729,22 @@ class CRDTTree: CRDTElement { } } + let parentOfNode = node.parent! + let previousNode = node.prevSibling ?? node.parent! + try changes.append(TreeChange(actor: editedAt.actorID, type: .removeStyle, - from: self.toIndex(fromParent, fromLeft), - to: self.toIndex(toParent, toLeft), - fromPath: self.toPath(fromParent, fromLeft), - toPath: self.toPath(toParent, toLeft), + from: self.toIndex(parentOfNode, previousNode), + to: self.toIndex(node, node), + fromPath: self.toPath(parentOfNode, previousNode), + toPath: self.toPath(node, node), value: value, splitLevel: 0) // dummy value. ) } } - return (pairs, changes) + return (createdAtMapByActor, pairs, changes) } /** @@ -730,7 +752,7 @@ class CRDTTree: CRDTElement { * If the content is undefined, the range will be removed. */ @discardableResult - func edit(_ range: TreePosRange, _ contents: [CRDTTreeNode]?, _ splitLevel: Int32, _ editedAt: TimeTicket, _ maxCreatedAtMapByActor: [String: TimeTicket] = [:], _ issueTimeTicket: () -> TimeTicket) throws -> ([TreeChange], [GCPair], [String: TimeTicket]) { + func edit(_ range: TreePosRange, _ contents: [CRDTTreeNode]?, _ splitLevel: Int32, _ editedAt: TimeTicket, _ issueTimeTicket: () -> TimeTicket, _ maxCreatedAtMapByActor: [String: TimeTicket]? = nil) throws -> ([TreeChange], [GCPair], [String: TimeTicket]) { // 01. find nodes from the given range and split nodes. let (fromParent, fromLeft) = try self.findNodesAndSplitText(range.0, editedAt) let (toParent, toLeft) = try self.findNodesAndSplitText(range.1, editedAt) @@ -760,7 +782,7 @@ class CRDTTree: CRDTElement { let actorID = node.createdAt.actorID - let maxCreatedAt = maxCreatedAtMapByActor.isEmpty == false ? maxCreatedAtMapByActor[actorID] ?? TimeTicket.initial : TimeTicket.max + let maxCreatedAt = maxCreatedAtMapByActor != nil ? maxCreatedAtMapByActor![actorID] ?? TimeTicket.initial : TimeTicket.max // NOTE(sejongk): If the node is removable or its parent is going to // be removed, then this node should be removed. @@ -886,7 +908,7 @@ class CRDTTree: CRDTElement { func editT(_ range: (Int, Int), _ contents: [CRDTTreeNode]?, _ splitLevel: Int32, _ editedAt: TimeTicket, _ issueTimeTicket: () -> TimeTicket) throws { let fromPos = try self.findPos(range.0) let toPos = try self.findPos(range.1) - try self.edit((fromPos, toPos), contents, splitLevel, editedAt, [:], issueTimeTicket) + try self.edit((fromPos, toPos), contents, splitLevel, editedAt, issueTimeTicket) } /** diff --git a/Sources/Document/CRDT/RHT.swift b/Sources/Document/CRDT/RHT.swift index ab5d5f92..d806d073 100644 --- a/Sources/Document/CRDT/RHT.swift +++ b/Sources/Document/CRDT/RHT.swift @@ -132,7 +132,7 @@ class RHT { * `get` returns the value of the given key. */ func get(key: String) throws -> String { - guard let node = self.nodeMapByKey[key] else { + guard self.has(key: key), let node = self.nodeMapByKey[key] else { let log = "can't find the given node with: \(key)" Logger.critical(log) throw YorkieError.unexpected(message: log) diff --git a/Sources/Document/Json/JSONTree.swift b/Sources/Document/Json/JSONTree.swift index faaa181e..d9cb2e18 100644 --- a/Sources/Document/Json/JSONTree.swift +++ b/Sources/Document/Json/JSONTree.swift @@ -384,7 +384,7 @@ public class JSONTree { let ticket = context.issueTimeTicket - let (maxCreationMapByActor, pairs, _) = try tree.style((fromPos, toPos), stringAttrs, ticket, nil) + let (maxCreationMapByActor, pairs, _) = try tree.style((fromPos, toPos), stringAttrs, ticket) context.push(operation: TreeStyleOperation(parentCreatedAt: tree.createdAt, fromPos: fromPos, @@ -442,7 +442,7 @@ public class JSONTree { let ticket = context.issueTimeTicket - let (pairs, _) = try tree.removeStyle((fromPos, toPos), attributesToRemove, ticket) + let (maxCreationMapByActor, pairs, _) = try tree.removeStyle((fromPos, toPos), attributesToRemove, ticket) for pair in pairs { self.context?.registerGCPair(pair) @@ -451,7 +451,7 @@ public class JSONTree { context.push(operation: TreeStyleOperation(parentCreatedAt: tree.createdAt, fromPos: fromPos, toPos: toPos, - maxCreatedAtMapByActor: [:], + maxCreatedAtMapByActor: maxCreationMapByActor, attributes: [:], attributesToRemove: attributesToRemove, executedAt: ticket) diff --git a/Sources/Document/Operation/Operation.swift b/Sources/Document/Operation/Operation.swift index b0e1cf46..98f0ee6d 100644 --- a/Sources/Document/Operation/Operation.swift +++ b/Sources/Document/Operation/Operation.swift @@ -194,13 +194,31 @@ public struct TreeEditOpInfo: OperationInfo { } } +public enum TreeStyleOpValue: Equatable { + case attributes([String: Any]) + case attributesToRemove([String]) + + public static func == (lhs: TreeStyleOpValue, rhs: TreeStyleOpValue) -> Bool { + switch (lhs, rhs) { + case (.attributes(let lhsAttributes), .attributes(let rhsAttributes)): + // Assuming [String: Any] comparison based on keys and string descriptions of values + return NSDictionary(dictionary: lhsAttributes).isEqual(to: rhsAttributes) + case (.attributesToRemove(let lhsAttributesToRemove), .attributesToRemove(let rhsAttributesToRemove)): + return lhsAttributesToRemove == rhsAttributesToRemove + default: + return false + } + } +} + public struct TreeStyleOpInfo: OperationInfo { public let type: OperationInfoType = .treeStyle public let path: String public let from: Int public let to: Int public let fromPath: [Int] - public let value: [String: Any] + public let toPath: [Int] + public let value: TreeStyleOpValue public static func == (lhs: TreeStyleOpInfo, rhs: TreeStyleOpInfo) -> Bool { if lhs.type != rhs.type { @@ -218,6 +236,9 @@ public struct TreeStyleOpInfo: OperationInfo { if lhs.fromPath != rhs.fromPath { return false } + if lhs.toPath != rhs.toPath { + return false + } if !(lhs.value == rhs.value) { return false diff --git a/Sources/Document/Operation/TreeEditOperation.swift b/Sources/Document/Operation/TreeEditOperation.swift index 7b858b91..fd80396d 100644 --- a/Sources/Document/Operation/TreeEditOperation.swift +++ b/Sources/Document/Operation/TreeEditOperation.swift @@ -71,7 +71,7 @@ struct TreeEditOperation: Operation { * Therefore, it is possible to simulate later timeTickets using `editedAt` and the length of `contents`. * This logic might be unclear; consider refactoring for multi-level concurrent editing in the Tree implementation. */ - let (changes, pairs, _) = try tree.edit((self.fromPos, self.toPos), self.contents?.compactMap { $0.deepcopy() }, self.splitLevel, editedAt, self.maxCreatedAtMapByActor) { + let (changes, pairs, _) = try tree.edit((self.fromPos, self.toPos), self.contents?.compactMap { $0.deepcopy() }, self.splitLevel, editedAt, { var delimiter = editedAt.delimiter if let contents { delimiter += UInt32(contents.count) @@ -81,7 +81,7 @@ struct TreeEditOperation: Operation { editedAt = TimeTicket(lamport: editedAt.lamport, delimiter: delimiter, actorID: editedAt.actorID) return editedAt - } + }, self.maxCreatedAtMapByActor) for pair in pairs { root.registerGCPair(pair) diff --git a/Sources/Document/Operation/TreeSytleOperation.swift b/Sources/Document/Operation/TreeSytleOperation.swift index 5a85c412..6532fc68 100644 --- a/Sources/Document/Operation/TreeSytleOperation.swift +++ b/Sources/Document/Operation/TreeSytleOperation.swift @@ -70,7 +70,7 @@ class TreeStyleOperation: Operation { if self.attributes.isEmpty == false { (_, pairs, changes) = try tree.style((self.fromPos, self.toPos), self.attributes, self.executedAt, self.maxCreatedAtMapByActor) } else { - (pairs, changes) = try tree.removeStyle((self.fromPos, self.toPos), self.attributesToRemove, self.executedAt) + (_, pairs, changes) = try tree.removeStyle((self.fromPos, self.toPos), self.attributesToRemove, self.executedAt, self.maxCreatedAtMapByActor) } guard let path = try? root.createPath(createdAt: parentCreatedAt) else { @@ -82,11 +82,14 @@ class TreeStyleOperation: Operation { } return changes.compactMap { change in - let attributes: [String: Any] = { - if case .attributes(let attributes) = change.value { - return attributes.toJSONObejct - } else { - return [:] + let values: TreeStyleOpValue? = { + switch change.value { + case .attributes(let attributes): + return .attributes(attributes.toJSONObejct) + case .attributesToRemove(let keys): + return .attributesToRemove(keys) + default: + return nil } }() @@ -95,7 +98,8 @@ class TreeStyleOperation: Operation { from: change.from, to: change.to, fromPath: change.fromPath, - value: attributes + toPath: change.toPath, + value: values! ) } } diff --git a/Sources/Util/IndexTree.swift b/Sources/Util/IndexTree.swift index 7fac482e..d813ad5d 100644 --- a/Sources/Util/IndexTree.swift +++ b/Sources/Util/IndexTree.swift @@ -208,6 +208,21 @@ extension IndexTreeNode { return parent.children[safe: offset + 1] } + /** + * `prevSibling` returns the previous sibling of the node. + */ + var prevSibling: Self? { + guard let parent else { + return nil + } + + guard let offset = try? parent.findOffset(node: self) else { + return nil + } + + return parent.children[safe: offset - 1] + } + /** * `splitText` splits the given node at the given offset. */ diff --git a/Tests/Integration/IntegrationHelper.swift b/Tests/Integration/IntegrationHelper.swift index 846a7ee4..d87fdd01 100644 --- a/Tests/Integration/IntegrationHelper.swift +++ b/Tests/Integration/IntegrationHelper.swift @@ -85,8 +85,9 @@ struct TreeEditOpInfoForDebug: OperationInfoForDebug { struct TreeStyleOpInfoForDebug: OperationInfoForDebug { let from: Int? let to: Int? - let value: [String: Codable]? + let value: TreeStyleOpValue? let fromPath: [Int]? + let toPath: [Int]? func compare(_ operation: TreeStyleOpInfo) { if let value = from { @@ -96,16 +97,14 @@ struct TreeStyleOpInfoForDebug: OperationInfoForDebug { XCTAssertEqual(value, operation.to) } if let value = value { - XCTAssertEqual(value.count, operation.value.count) - if operation.value.count - 1 >= 0 { - for key in operation.value.keys { - XCTAssertEqual(value[key]?.toJSONString, convertToJSONString(operation.value[key] ?? NSNull())) - } - } + XCTAssertEqual(value, operation.value) } if let value = fromPath { XCTAssertEqual(value, operation.fromPath) } + if let value = toPath { + XCTAssertEqual(value, operation.toPath) + } } } diff --git a/Tests/Integration/TreeConcurrencyTests.swift b/Tests/Integration/TreeConcurrencyTests.swift index de6429cc..99cfabd5 100644 --- a/Tests/Integration/TreeConcurrencyTests.swift +++ b/Tests/Integration/TreeConcurrencyTests.swift @@ -301,12 +301,11 @@ final class TreeConcurrencyTests: XCTestCase { DispatchQueue.global().async { Task { let result = try await self.runTest(initialState: initialState.deepcopy(), initialXML: initialXML, ranges: ranges, op1: op1, op2: op2, desc: desc) - print("====== before d1: \(result.before.0)") print("====== before d2: \(result.before.1)") print("====== after d1: \(result.after.0)") print("====== after d2: \(result.after.1)") - XCTAssertEqual(result.after.0, result.after.1) + XCTAssertEqual(result.after.0, result.after.1, desc) exp.fulfill() } @@ -609,7 +608,7 @@ final class TreeConcurrencyTests: XCTestCase { ) let initialXML = "

a

b

c

" - let content = JSONTreeElementNode(type: "p", children: [JSONTreeTextNode(value: "d")], attributes: ["italic": true]) + let content = JSONTreeElementNode(type: "p", children: [JSONTreeTextNode(value: "d")], attributes: ["italic": true, "color": "blue"]) let rangesArr = [ // equal:

b

-

b

@@ -679,7 +678,7 @@ final class TreeConcurrencyTests: XCTestCase { .styleRemove, "color", "", - "remove-bold" + "remove-color" ), StyleOperationType( .rangeAll, diff --git a/Tests/Integration/TreeIntegrationTests.swift b/Tests/Integration/TreeIntegrationTests.swift index a9b9d0a1..606a8ef1 100644 --- a/Tests/Integration/TreeIntegrationTests.swift +++ b/Tests/Integration/TreeIntegrationTests.swift @@ -3722,6 +3722,104 @@ final class TreeIntegrationTreeChangeGeneration: XCTestCase { } } + func test_concurrent_insert_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: []) + ]) + ) + } + + 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, /* html */ "

") + XCTAssertEqual(d1XML, d2XML) + + await subscribeDocs(d1, + d2, + [TreeStyleOpInfoForDebug(from: 0, to: 1, value: .attributes(["key": "a"]), fromPath: [0], toPath: [0, 0]), + TreeStyleOpInfoForDebug(from: 0, to: 1, value: .attributes(["key": "a"]), fromPath: [0], toPath: [0, 0]), + TreeEditOpInfoForDebug(from: 0, to: 0, value: [JSONTreeElementNode(type: "p")], fromPath: [0], toPath: [0])], + [TreeEditOpInfoForDebug(from: 0, to: 0, value: [JSONTreeElementNode(type: "p")], fromPath: [0], toPath: [0]), + TreeStyleOpInfoForDebug(from: 2, to: 3, value: .attributes(["key": "a"]), fromPath: [1], toPath: [1, 0]), + TreeStyleOpInfoForDebug(from: 2, to: 3, value: .attributes(["key": "a"]), fromPath: [1], toPath: [1, 0])]) + + try await d1.update { root, _ in + try (root.t as? JSONTree)?.style(0, 1, ["key": "a"]) + } + try await d1.update { root, _ in + try (root.t as? JSONTree)?.style(0, 1, ["key": "a"]) + } + try await d2.update { root, _ in + try (root.t as? JSONTree)?.edit(0, 0, JSONTreeElementNode(type: "p")) + } + + 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, /* html */ "

") + XCTAssertEqual(d1XML, d2XML) + } + } + + func test_concurrent_insert_and_removeStyle() 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: [], attributes: ["key": "a"]) + ]) + ) + } + + 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, /* html */ "

") + XCTAssertEqual(d1XML, d2XML) + + await subscribeDocs(d1, + d2, + [TreeStyleOpInfoForDebug(from: 0, to: 1, value: .attributesToRemove(["key"]), fromPath: [0], toPath: [0, 0]), + TreeStyleOpInfoForDebug(from: 0, to: 1, value: .attributesToRemove(["key"]), fromPath: [0], toPath: [0, 0]), + TreeEditOpInfoForDebug(from: 0, to: 0, value: [JSONTreeElementNode(type: "p")], fromPath: [0], toPath: [0])], + [TreeEditOpInfoForDebug(from: 0, to: 0, value: [JSONTreeElementNode(type: "p")], fromPath: [0], toPath: [0]), + TreeStyleOpInfoForDebug(from: 2, to: 3, value: .attributesToRemove(["key"]), fromPath: [1], toPath: [1, 0]), + TreeStyleOpInfoForDebug(from: 2, to: 3, value: .attributesToRemove(["key"]), fromPath: [1], toPath: [1, 0])]) + + try await d1.update { root, _ in + try (root.t as? JSONTree)?.removeStyle(0, 1, ["key"]) + } + try await d1.update { root, _ in + try (root.t as? JSONTree)?.removeStyle(0, 1, ["key"]) + } + try await d2.update { root, _ in + try (root.t as? JSONTree)?.edit(0, 0, JSONTreeElementNode(type: "p")) + } + + 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, /* html */ "

") + XCTAssertEqual(d1XML, d2XML) + } + } + func test_concurrent_delete_and_style() async throws { try await withTwoClientsAndDocuments(self.description) { c1, d1, c2, d2 in try await d1.update { root, _ in @@ -3742,7 +3840,7 @@ final class TreeIntegrationTreeChangeGeneration: XCTestCase { await subscribeDocs(d1, d2, - [TreeStyleOpInfoForDebug(from: 0, to: 1, value: ["value": "changed"], fromPath: nil), + [TreeStyleOpInfoForDebug(from: 0, to: 1, value: .attributes(["value": "changed"]), fromPath: nil, toPath: nil), TreeEditOpInfoForDebug(from: 0, to: 2, value: nil, fromPath: nil, toPath: nil)], [TreeEditOpInfoForDebug(from: 0, to: 2, value: nil, fromPath: nil, toPath: nil)]) @@ -3789,9 +3887,9 @@ final class TreeIntegrationTreeChangeGeneration: XCTestCase { 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)]) + [TreeStyleOpInfoForDebug(from: 0, to: 1, value: .attributes(["bold": "true"]), fromPath: nil, toPath: nil), + TreeStyleOpInfoForDebug(from: 0, to: 1, value: .attributes(["bold": "false"]), fromPath: nil, toPath: nil)], + [TreeStyleOpInfoForDebug(from: 0, to: 1, value: .attributes(["bold": "false"]), fromPath: nil, toPath: nil)]) try await d1.update { root, _ in try (root.t as? JSONTree)?.style(0, 1, ["bold": "true"]) From bbf205864001ec95e540581dabfbbddec1fb7493 Mon Sep 17 00:00:00 2001 From: Jung gyun Ahn Date: Wed, 12 Jun 2024 01:32:28 +0900 Subject: [PATCH 2/6] Update swift-integration.yml --- .github/workflows/swift-integration.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/swift-integration.yml b/.github/workflows/swift-integration.yml index ecec2012..99ba7ac7 100644 --- a/.github/workflows/swift-integration.yml +++ b/.github/workflows/swift-integration.yml @@ -15,7 +15,7 @@ jobs: xcode-version: latest-stable - uses: actions/checkout@v4 - name: Setup yorkie server - run: brew install yorkie + run: brew reinstall yorkie - name: Run yorkie server run: yorkie server & - name: Run tests From 75aad2ea4e1928d2b9e1c30a8ec6f5f58572fe64 Mon Sep 17 00:00:00 2001 From: Jung gyun Ahn Date: Wed, 12 Jun 2024 01:36:10 +0900 Subject: [PATCH 3/6] Update swift-integration.yml --- .github/workflows/swift-integration.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/swift-integration.yml b/.github/workflows/swift-integration.yml index 99ba7ac7..50861888 100644 --- a/.github/workflows/swift-integration.yml +++ b/.github/workflows/swift-integration.yml @@ -14,9 +14,11 @@ jobs: with: xcode-version: latest-stable - uses: actions/checkout@v4 + - name: Update Brew + run: brew update - name: Setup yorkie server run: brew reinstall yorkie - name: Run yorkie server - run: yorkie server & + run: brew services start yorkie - name: Run tests run: swift test --enable-code-coverage -v --filter YorkieIntegrationTests From bcbb1b208562ebfff9fb45107d6f15bf7e7e5cf4 Mon Sep 17 00:00:00 2001 From: Jung gyun Ahn Date: Wed, 12 Jun 2024 01:39:18 +0900 Subject: [PATCH 4/6] Update swift-integration.yml --- .github/workflows/swift-integration.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/swift-integration.yml b/.github/workflows/swift-integration.yml index 50861888..30def2b7 100644 --- a/.github/workflows/swift-integration.yml +++ b/.github/workflows/swift-integration.yml @@ -19,6 +19,6 @@ jobs: - name: Setup yorkie server run: brew reinstall yorkie - name: Run yorkie server - run: brew services start yorkie + run: yorkie server & - name: Run tests run: swift test --enable-code-coverage -v --filter YorkieIntegrationTests From 969e8a91245a6f230f52494645e71a97fe1b3903 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EC=95=88=EC=A0=95=EA=B7=A0?= Date: Wed, 12 Jun 2024 02:06:41 +0900 Subject: [PATCH 5/6] Add RHTNode removal to converter for consistency --- Sources/API/Converter.swift | 5 +++-- .../API/V1/Generated/yorkie/v1/resources.pb.swift | 8 ++++++++ Sources/API/V1/yorkie/v1/resources.proto | 1 + Sources/Document/CRDT/RHT.swift | 14 +++++++++++++- Tests/Unit/API/V1/ConverterTests.swift | 10 +++++++++- Tests/Unit/Document/CRDT/RHTTests.swift | 10 ++++++++++ 6 files changed, 44 insertions(+), 4 deletions(-) diff --git a/Sources/API/Converter.swift b/Sources/API/Converter.swift index 000dd9c5..ad72f476 100644 --- a/Sources/API/Converter.swift +++ b/Sources/API/Converter.swift @@ -900,7 +900,7 @@ extension Converter { return pbTreeNodes } } - + /** * `toTreeNodes` converts the given model to Protobuf format. */ @@ -935,6 +935,7 @@ extension Converter { var pbNodeAttr = PbNodeAttr() pbNodeAttr.value = rhtNode.value pbNodeAttr.updatedAt = toTimeTicket(rhtNode.updatedAt) + pbNodeAttr.isRemoved = rhtNode.isRemoved pbTreeNode.attributes[rhtNode.key] = pbNodeAttr } @@ -1012,7 +1013,7 @@ extension Converter { node.attrs = RHT() pbTreeNode.attributes.forEach { key, value in - node.attrs?.set(key: key, value: value.value, executedAt: fromTimeTicket(value.updatedAt)) + node.attrs?.setInternal(key: key, value: value.value, executedAt: fromTimeTicket(value.updatedAt), removed: value.isRemoved) } } diff --git a/Sources/API/V1/Generated/yorkie/v1/resources.pb.swift b/Sources/API/V1/Generated/yorkie/v1/resources.pb.swift index 159d9fda..2e618511 100644 --- a/Sources/API/V1/Generated/yorkie/v1/resources.pb.swift +++ b/Sources/API/V1/Generated/yorkie/v1/resources.pb.swift @@ -1452,6 +1452,8 @@ public struct Yorkie_V1_NodeAttr { /// Clears the value of `updatedAt`. Subsequent reads from it will return its default value. public mutating func clearUpdatedAt() {self._updatedAt = nil} + public var isRemoved: Bool = false + public var unknownFields = SwiftProtobuf.UnknownStorage() public init() {} @@ -4133,6 +4135,7 @@ extension Yorkie_V1_NodeAttr: SwiftProtobuf.Message, SwiftProtobuf._MessageImple public static let _protobuf_nameMap: SwiftProtobuf._NameMap = [ 1: .same(proto: "value"), 2: .standard(proto: "updated_at"), + 3: .standard(proto: "is_removed"), ] public mutating func decodeMessage(decoder: inout D) throws { @@ -4143,6 +4146,7 @@ extension Yorkie_V1_NodeAttr: SwiftProtobuf.Message, SwiftProtobuf._MessageImple switch fieldNumber { case 1: try { try decoder.decodeSingularStringField(value: &self.value) }() case 2: try { try decoder.decodeSingularMessageField(value: &self._updatedAt) }() + case 3: try { try decoder.decodeSingularBoolField(value: &self.isRemoved) }() default: break } } @@ -4159,12 +4163,16 @@ extension Yorkie_V1_NodeAttr: SwiftProtobuf.Message, SwiftProtobuf._MessageImple try { if let v = self._updatedAt { try visitor.visitSingularMessageField(value: v, fieldNumber: 2) } }() + if self.isRemoved != false { + try visitor.visitSingularBoolField(value: self.isRemoved, fieldNumber: 3) + } try unknownFields.traverse(visitor: &visitor) } public static func ==(lhs: Yorkie_V1_NodeAttr, rhs: Yorkie_V1_NodeAttr) -> Bool { if lhs.value != rhs.value {return false} if lhs._updatedAt != rhs._updatedAt {return false} + if lhs.isRemoved != rhs.isRemoved {return false} if lhs.unknownFields != rhs.unknownFields {return false} return true } diff --git a/Sources/API/V1/yorkie/v1/resources.proto b/Sources/API/V1/yorkie/v1/resources.proto index a93eb7f4..d1a8c377 100644 --- a/Sources/API/V1/yorkie/v1/resources.proto +++ b/Sources/API/V1/yorkie/v1/resources.proto @@ -226,6 +226,7 @@ message RGANode { message NodeAttr { string value = 1; TimeTicket updated_at = 2; + bool is_removed = 3; } message TextNode { diff --git a/Sources/Document/CRDT/RHT.swift b/Sources/Document/CRDT/RHT.swift index d806d073..946d22fa 100644 --- a/Sources/Document/CRDT/RHT.swift +++ b/Sources/Document/CRDT/RHT.swift @@ -84,6 +84,18 @@ class RHT { return (prev?.isRemoved ?? false ? prev : nil, nil) } + /** + * SetInternal sets the value of the given key internally. + */ + func setInternal(key: String, value: String, executedAt: TimeTicket, removed: Bool) { + let node = RHTNode(key: key, value: value, updatedAt: executedAt, isRemoved: removed) + self.nodeMapByKey[key] = node + + if removed { + self.numberOfRemovedElement += 1 + } + } + /** * `remove` removes the Element of the given key. */ @@ -147,7 +159,7 @@ class RHT { func deepcopy() -> RHT { let rht = RHT() self.nodeMapByKey.forEach { - rht.set(key: $1.key, value: $1.value, executedAt: $1.updatedAt) + rht.setInternal(key: $1.key, value: $1.value, executedAt: $1.updatedAt, removed: $1.isRemoved) } return rht } diff --git a/Tests/Unit/API/V1/ConverterTests.swift b/Tests/Unit/API/V1/ConverterTests.swift index 50dbca21..bd5aaf51 100644 --- a/Tests/Unit/API/V1/ConverterTests.swift +++ b/Tests/Unit/API/V1/ConverterTests.swift @@ -376,12 +376,20 @@ class ConverterTests: XCTestCase { ])) try (root.tree as? JSONTree)?.editByPath([0, 1], [1, 1]) + + try (root.tree as? JSONTree)?.style(0, 1, ["b": "t", "i": "t"]) + + let xml = (root.tree as? JSONTree)?.toXML() + + XCTAssertEqual(xml, "

14

") + + try (root.tree as? JSONTree)?.removeStyle(0, 1, ["i"]) } let xml = await(doc.getRoot().tree as? JSONTree)?.toXML() let size = try await(doc.getRoot().tree as? JSONTree)?.getSize() - XCTAssertEqual(xml, "

14

") + XCTAssertEqual(xml, "

14

") XCTAssertEqual(size, 4) let bytes = try await Converter.objectToBytes(obj: doc.getRootObject()) diff --git a/Tests/Unit/Document/CRDT/RHTTests.swift b/Tests/Unit/Document/CRDT/RHTTests.swift index 1df931c1..2dc6db2e 100644 --- a/Tests/Unit/Document/CRDT/RHTTests.swift +++ b/Tests/Unit/Document/CRDT/RHTTests.swift @@ -156,6 +156,16 @@ class RHTTests: XCTestCase { XCTAssertEqual(result[key]?.value, value) } } + + func test_should_deepcopy_correctly() { + let rht = RHT() + rht.set(key: "key1", value: "value1", executedAt: timeT()) + rht.remove(key: "key2", executedAt: timeT()) + + let rht2 = rht.deepcopy() + XCTAssertEqual(rht.toJSON(), rht2.toJSON()) + XCTAssertEqual(rht.size, rht2.size) + } } final class GCTestsForRHT: XCTestCase { From aeff425dcdb17463a45cb6410a0dbc13e4855ae7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EC=95=88=EC=A0=95=EA=B7=A0?= Date: Wed, 12 Jun 2024 02:45:12 +0900 Subject: [PATCH 6/6] Fix miscalculation of tree size in concurrent editing --- Sources/Document/CRDT/CRDTTree.swift | 6 +- Sources/Util/IndexTree.swift | 19 +-- Tests/Integration/TreeIntegrationTests.swift | 137 +++++++++++++++++++ 3 files changed, 149 insertions(+), 13 deletions(-) diff --git a/Sources/Document/CRDT/CRDTTree.swift b/Sources/Document/CRDT/CRDTTree.swift index 75d91b50..32ddec48 100644 --- a/Sources/Document/CRDT/CRDTTree.swift +++ b/Sources/Document/CRDT/CRDTTree.swift @@ -351,11 +351,7 @@ final class CRDTTreeNode: IndexTreeNode { } if alived { - if self.parent?.removedAt == nil { - self.updateAncestorsSize() - } else { - self.parent?.size -= self.paddedSize - } + self.updateAncestorsSize() } } diff --git a/Sources/Util/IndexTree.swift b/Sources/Util/IndexTree.swift index d813ad5d..c13b6907 100644 --- a/Sources/Util/IndexTree.swift +++ b/Sources/Util/IndexTree.swift @@ -145,6 +145,9 @@ extension IndexTreeNode { while parent != nil { parent?.size += self.paddedSize * sign + if parent!.isRemoved { + break + } parent = parent?.parent } } @@ -155,17 +158,17 @@ extension IndexTreeNode { */ @discardableResult func updateDescendantsSize() -> Int { - if self.isRemoved { - self.size = 0 - return 0 - } - - var sum = 0 + var size = 0 for child in self.children { - sum += child.updateDescendantsSize() + let childSize = child.updateDescendantsSize() + if child.isRemoved { + continue + } + + size += childSize } - self.size += sum + self.size += size return self.paddedSize } diff --git a/Tests/Integration/TreeIntegrationTests.swift b/Tests/Integration/TreeIntegrationTests.swift index 606a8ef1..90f0cd26 100644 --- a/Tests/Integration/TreeIntegrationTests.swift +++ b/Tests/Integration/TreeIntegrationTests.swift @@ -3248,6 +3248,143 @@ final class TreeIntegrationEdgeCases: XCTestCase { } } + func test_can_calculate_size_of_index_tree_correctly_during_concurrent_editing() 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, /* html */ "

hello

") + XCTAssertEqual(d2XML, /* html */ "

hello

") + + try await d1.update { root, _ in + try (root.t as? JSONTree)?.edit(0, 7) + } + + try await d2.update { root, _ in + try (root.t as? JSONTree)?.edit(1, 2, JSONTreeTextNode(value: "p")) + } + + d1XML = await(d1.getRoot().t as? JSONTree)?.toXML() + XCTAssertEqual(d1XML, /* html */ "") + + var sized1 = try await(d1.getRoot().t as? JSONTree)?.getSize() + XCTAssertEqual(0, sized1) + + d2XML = await(d2.getRoot().t as? JSONTree)?.toXML() + XCTAssertEqual(d2XML, /* html */ "

pello

") + + var sized2 = try await(d2.getRoot().t as? JSONTree)?.getSize() + XCTAssertEqual(7, sized2) + + 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, /* html */ "") + XCTAssertEqual(d2XML, /* html */ "") + + sized1 = try await(d1.getRoot().t as? JSONTree)?.getSize() + sized2 = try await(d2.getRoot().t as? JSONTree)?.getSize() + XCTAssertEqual(sized1, sized2) + } + } + + func test_can_keep_index_tree_consistent_from_snapshot() async throws { + try await withTwoClientsAndDocuments(self.description) { c1, d1, c2, d2 in + try await d1.update { root, _ in + root.t = JSONTree(initialRoot: + JSONTreeElementNode(type: "r", + children: [ + JSONTreeElementNode(type: "p", children: []) + ]) + ) + } + + 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, /* html */ "

") + XCTAssertEqual(d2XML, /* html */ "

") + + try await d1.update { root, _ in + try (root.t as? JSONTree)?.edit(0, 2) + } + + try await d2.update { root, _ in + try (root.t as? JSONTree)?.edit(1, 1, JSONTreeElementNode(type: "i", children: [JSONTreeTextNode(value: "a")])) + try (root.t as? JSONTree)?.edit(2, 3, JSONTreeTextNode(value: "b")) + } + + d1XML = await(d1.getRoot().t as? JSONTree)?.toXML() + XCTAssertEqual(d1XML, /* html */ "") + let sized1 = try await(d1.getRoot().t as? JSONTree)?.getSize() + XCTAssertEqual(0, sized1) + d2XML = await(d2.getRoot().t as? JSONTree)?.toXML() + XCTAssertEqual(d2XML, /* html */ "

b

") + let sized2 = try await(d2.getRoot().t as? JSONTree)?.getSize() + XCTAssertEqual(5, sized2) + + try await c1.sync() + try await c2.sync() + try await c1.sync() + + d1XML = await(d1.getRoot().t as? JSONTree)?.toXML() + XCTAssertEqual(d1XML, /* html */ "") + d2XML = await(d2.getRoot().t as? JSONTree)?.toXML() + XCTAssertEqual(d1XML, /* html */ "") + + var d1Nodes = [(String, Int, Bool)]() + var d2Nodes = [(String, Int, Bool)]() + var sNodes = [(String, Int, Bool)]() + + try await(d1.getRoot().t as? JSONTree)?.getIndexTree().traverseAll { node, _ in + d1Nodes.append((node.toJSONString, node.size, node.isRemoved)) + } + try await(d2.getRoot().t as? JSONTree)?.getIndexTree().traverseAll { node, _ in + d2Nodes.append((node.toJSONString, node.size, node.isRemoved)) + } + + let sRoot = try await Converter.bytesToObject(bytes: Converter.objectToBytes(obj: d1.getRootObject())) + + (sRoot.get(key: "t") as? CRDTTree)?.indexTree.traverseAll { node, _ in + sNodes.append((node.toJSONString, node.size, node.isRemoved)) + } + + XCTAssertEqual(d1Nodes.count, d2Nodes.count) + + for index in 0 ..< d1Nodes.count { + XCTAssertEqual(d1Nodes[index].0, d2Nodes[index].0) + XCTAssertEqual(d1Nodes[index].1, d2Nodes[index].1) + XCTAssertEqual(d1Nodes[index].2, d2Nodes[index].2) + } + + XCTAssertEqual(d1Nodes.count, sNodes.count) + + for index in 0 ..< d1Nodes.count { + XCTAssertEqual(d1Nodes[index].0, sNodes[index].0) + XCTAssertEqual(d1Nodes[index].1, sNodes[index].1) + XCTAssertEqual(d1Nodes[index].2, sNodes[index].2) + } + } + } + func test_can_split_and_merge_with_empty_paragraph_left() async throws { try await withTwoClientsAndDocuments(self.description) { c1, d1, c2, d2 in try await d1.update { root, _ in