From 21f6b2f1e018dc3882b10af8538bc1422545a158 Mon Sep 17 00:00:00 2001 From: Yourim Cha Date: Fri, 17 Nov 2023 17:49:36 +0900 Subject: [PATCH 1/9] Modify set and add operations to allow setting initial values for object and array Previously, only the outer shells of object ({}) and array ([]) were added, requiring separate operations to set their internal values. The modification enables setting initial values directly within a single operation. --- src/api/converter.ts | 21 ++++++++- src/document/crdt/array.ts | 15 +++++- src/document/crdt/object.ts | 15 +++++- src/document/json/array.ts | 51 ++++++++++++++++----- src/document/json/object.ts | 61 +++++++++++++++++++++---- src/document/operation/add_operation.ts | 2 +- src/document/operation/set_operation.ts | 14 ++++-- test/integration/document_test.ts | 44 +++--------------- test/integration/object_test.ts | 9 ++-- 9 files changed, 159 insertions(+), 73 deletions(-) diff --git a/src/api/converter.ts b/src/api/converter.ts index 7e5b07a92..e3b59f56b 100644 --- a/src/api/converter.ts +++ b/src/api/converter.ts @@ -208,9 +208,11 @@ function toElementSimple(element: CRDTElement): PbJSONElementSimple { if (element instanceof CRDTObject) { pbElementSimple.setType(PbValueType.VALUE_TYPE_JSON_OBJECT); pbElementSimple.setCreatedAt(toTimeTicket(element.getCreatedAt())); + pbElementSimple.setValue(objectToBytes(element)); } else if (element instanceof CRDTArray) { pbElementSimple.setType(PbValueType.VALUE_TYPE_JSON_ARRAY); pbElementSimple.setCreatedAt(toTimeTicket(element.getCreatedAt())); + pbElementSimple.setValue(arrayToBytes(element)); } else if (element instanceof CRDTText) { pbElementSimple.setType(PbValueType.VALUE_TYPE_TEXT); pbElementSimple.setCreatedAt(toTimeTicket(element.getCreatedAt())); @@ -835,9 +837,9 @@ function fromCounterType(pbValueType: PbValueType): CounterType { function fromElementSimple(pbElementSimple: PbJSONElementSimple): CRDTElement { switch (pbElementSimple.getType()) { case PbValueType.VALUE_TYPE_JSON_OBJECT: - return CRDTObject.create(fromTimeTicket(pbElementSimple.getCreatedAt())!); + return bytesToObject(pbElementSimple.getValue_asU8()); case PbValueType.VALUE_TYPE_JSON_ARRAY: - return CRDTArray.create(fromTimeTicket(pbElementSimple.getCreatedAt())!); + return bytesToArray(pbElementSimple.getValue_asU8()); case PbValueType.VALUE_TYPE_TEXT: return CRDTText.create( RGATreeSplit.create(), @@ -1357,6 +1359,21 @@ function objectToBytes(obj: CRDTObject): Uint8Array { return toElement(obj).serializeBinary(); } +/** + * `bytesToArray` creates an CRDTArray from the given bytes. + */ +function bytesToArray(bytes: Uint8Array): CRDTArray { + const pbElement = PbJSONElement.deserializeBinary(bytes); + return fromArray(pbElement.getJsonArray()!); +} + +/** + * `arrayToBytes` converts the given CRDTArray to bytes. + */ +function arrayToBytes(array: CRDTArray): Uint8Array { + return toArray(array).serializeBinary(); +} + /** * `bytesToTree` creates an CRDTTree from the given bytes. */ diff --git a/src/document/crdt/array.ts b/src/document/crdt/array.ts index 9ad8513c2..8a930d4e9 100644 --- a/src/document/crdt/array.ts +++ b/src/document/crdt/array.ts @@ -39,8 +39,19 @@ export class CRDTArray extends CRDTContainer { /** * `create` creates a new instance of Array. */ - public static create(createdAt: TimeTicket): CRDTArray { - return new CRDTArray(createdAt, RGATreeList.create()); + public static create( + createdAt: TimeTicket, + value?: Array, + ): CRDTArray { + if (!value) { + return new CRDTArray(createdAt, RGATreeList.create()); + } + + const elements = RGATreeList.create(); + for (const v of value) { + elements.insertAfter(elements.getLastCreatedAt(), v.deepcopy()); + } + return new CRDTArray(createdAt, elements); } /** diff --git a/src/document/crdt/object.ts b/src/document/crdt/object.ts index 86b650a61..a9ca13ee6 100644 --- a/src/document/crdt/object.ts +++ b/src/document/crdt/object.ts @@ -39,8 +39,19 @@ export class CRDTObject extends CRDTContainer { /** * `create` creates a new instance of CRDTObject. */ - public static create(createdAt: TimeTicket): CRDTObject { - return new CRDTObject(createdAt, ElementRHT.create()); + public static create( + createdAt: TimeTicket, + value?: { [key: string]: CRDTElement }, + ): CRDTObject { + if (!value) { + return new CRDTObject(createdAt, ElementRHT.create()); + } + + const memberNodes = ElementRHT.create(); + for (const [k, v] of Object.entries(value)) { + memberNodes.set(k, v.deepcopy(), v.getCreatedAt()); + } + return new CRDTObject(createdAt, memberNodes); } /** diff --git a/src/document/json/array.ts b/src/document/json/array.ts index b8e45b1d2..8b30e364c 100644 --- a/src/document/json/array.ts +++ b/src/document/json/array.ts @@ -20,7 +20,10 @@ import { AddOperation } from '@yorkie-js-sdk/src/document/operation/add_operatio import { MoveOperation } from '@yorkie-js-sdk/src/document/operation/move_operation'; import { RemoveOperation } from '@yorkie-js-sdk/src/document/operation/remove_operation'; import { ChangeContext } from '@yorkie-js-sdk/src/document/change/context'; -import { CRDTElement } from '@yorkie-js-sdk/src/document/crdt/element'; +import { + CRDTContainer, + CRDTElement, +} from '@yorkie-js-sdk/src/document/crdt/element'; import { CRDTObject } from '@yorkie-js-sdk/src/document/crdt/object'; import { CRDTArray } from '@yorkie-js-sdk/src/document/crdt/array'; import { @@ -329,6 +332,31 @@ export class ArrayProxy { } } + /** + * `buildArray` constructs an array of CRDTElements based on the user-provided array. + */ + public static buildArray( + context: ChangeContext, + value: Array, + ): Array { + const elementArray: Array = []; + for (const v of value) { + const ticket = context.issueTimeTicket(); + let elem: CRDTElement; + if (Primitive.isSupport(v)) { + elem = Primitive.of(v as PrimitiveValue, ticket); + } else if (Array.isArray(v)) { + elem = CRDTArray.create(ticket, ArrayProxy.buildArray(context, v)); + } else if (typeof v === 'object') { + elem = CRDTObject.create(ticket, ObjectProxy.buildObject(context, v!)); + } else { + throw new TypeError(`Unsupported type of value: ${typeof value}`); + } + elementArray.push(elem); + } + return elementArray; + } + /** * `pushInternal` pushes the value to the target array. */ @@ -455,7 +483,10 @@ export class ArrayProxy { ); return primitive; } else if (Array.isArray(value)) { - const array = CRDTArray.create(ticket); + const array = CRDTArray.create( + ticket, + ArrayProxy.buildArray(context, value), + ); const clone = array.deepcopy(); target.insertAfter(prevCreatedAt, clone); context.registerElement(clone, target); @@ -467,12 +498,12 @@ export class ArrayProxy { ticket, ), ); - for (const element of value) { - ArrayProxy.pushInternal(context, clone, element); - } return array; } else if (typeof value === 'object') { - const obj = CRDTObject.create(ticket); + const obj = CRDTObject.create( + ticket, + ObjectProxy.buildObject(context, value!), + ); target.insertAfter(prevCreatedAt, obj); context.registerElement(obj, target); context.push( @@ -483,10 +514,6 @@ export class ArrayProxy { ticket, ), ); - - for (const [k, v] of Object.entries(value!)) { - ObjectProxy.setInternal(context, obj, k, v); - } return obj; } @@ -579,7 +606,9 @@ export class ArrayProxy { for (let i = from; i < to; i++) { const removed = ArrayProxy.deleteInternalByIndex(context, target, from); if (removed) { - removeds.push(toJSONElement(context, removed)!); + const removedElem = removed.deepcopy(); + removedElem.setRemovedAt(); + removeds.push(toJSONElement(context, removedElem)!); } } if (items) { diff --git a/src/document/json/object.ts b/src/document/json/object.ts index 8755b65ee..603d5fff0 100644 --- a/src/document/json/object.ts +++ b/src/document/json/object.ts @@ -172,7 +172,14 @@ export class ObjectProxy { SetOperation.create(key, primitive, target.getCreatedAt(), ticket), ); } else if (Array.isArray(value)) { - const array = CRDTArray.create(ticket); + const array = CRDTArray.create( + ticket, + ArrayProxy.buildArray(context, value), + ); + array.getDescendants((elem, parent) => { + context.registerElement(elem, parent); + return false; + }); setAndRegister(array); context.push( SetOperation.create( @@ -182,9 +189,6 @@ export class ObjectProxy { ticket, ), ); - for (const element of value) { - ArrayProxy.pushInternal(context, array, element); - } } else if (typeof value === 'object') { if (value instanceof Text) { const text = CRDTText.create(RGATreeSplit.create(), ticket); @@ -230,7 +234,14 @@ export class ObjectProxy { ); value.initialize(context, tree); } else { - const obj = CRDTObject.create(ticket); + const obj = CRDTObject.create( + ticket, + ObjectProxy.buildObject(context, value!), + ); + obj.getDescendants((elem, parent) => { + context.registerElement(elem, parent); + return false; + }); setAndRegister(obj); context.push( SetOperation.create( @@ -240,15 +251,49 @@ export class ObjectProxy { ticket, ), ); - for (const [k, v] of Object.entries(value!)) { - ObjectProxy.setInternal(context, obj, k, v); - } } } else { logger.fatal(`unsupported type of value: ${typeof value}`); } } + /** + * `buildObject` constructs an object where all values from the + * user-provided object are transformed into CRDTElements. + * This function takes an object and iterates through its values, + * converting each value into a corresponding CRDTElement. + */ + public static buildObject( + context: ChangeContext, + value: object, + ): { [key: string]: CRDTElement } { + const elementObject: { [key: string]: CRDTElement } = {}; + for (const [k, v] of Object.entries(value)) { + if (k.includes('.')) { + throw new YorkieError( + Code.InvalidObjectKey, + `key must not contain the '.'.`, + ); + } + + const ticket = context.issueTimeTicket(); + let elem: CRDTElement; + if (Primitive.isSupport(v)) { + elem = Primitive.of(v as PrimitiveValue, ticket); + } else if (Array.isArray(v)) { + const array = ArrayProxy.buildArray(context, v); + elem = CRDTArray.create(ticket, array); + } else if (typeof v === 'object') { + const object = ObjectProxy.buildObject(context, v); + elem = CRDTObject.create(ticket, object); + } else { + throw new TypeError(`Unsupported type of value: ${typeof value}`); + } + elementObject[k] = elem; + } + return elementObject; + } + /** * `deleteInternal` deletes the value of the given key. */ diff --git a/src/document/operation/add_operation.ts b/src/document/operation/add_operation.ts index 8a732e034..6412f380b 100644 --- a/src/document/operation/add_operation.ts +++ b/src/document/operation/add_operation.ts @@ -91,7 +91,7 @@ export class AddOperation extends Operation { * `toTestString` returns a string containing the meta data. */ public toTestString(): string { - return `${this.getParentCreatedAt().toTestString()}.ADD`; + return `${this.getParentCreatedAt().toTestString()}.ADD.${this.value.toJSON()}`; } /** diff --git a/src/document/operation/set_operation.ts b/src/document/operation/set_operation.ts index 74ae9eb91..80ad93dd4 100644 --- a/src/document/operation/set_operation.ts +++ b/src/document/operation/set_operation.ts @@ -16,7 +16,10 @@ import { logger } from '@yorkie-js-sdk/src/util/logger'; import { TimeTicket } from '@yorkie-js-sdk/src/document/time/ticket'; -import { CRDTElement } from '@yorkie-js-sdk/src/document/crdt/element'; +import { + CRDTContainer, + CRDTElement, +} from '@yorkie-js-sdk/src/document/crdt/element'; import { CRDTRoot } from '@yorkie-js-sdk/src/document/crdt/root'; import { CRDTObject } from '@yorkie-js-sdk/src/document/crdt/object'; import { @@ -85,6 +88,12 @@ export class SetOperation extends Operation { if (removed) { root.registerRemovedElement(removed); } + if (value instanceof CRDTContainer) { + value.getDescendants((elem, parent) => { + root.registerElement(elem, parent); + return false; + }); + } return { opInfos: [ @@ -108,9 +117,6 @@ export class SetOperation extends Operation { ); if (value !== undefined && !value.isRemoved()) { - // TODO(chacha912): When the value is an object, - // it always sets as an empty object from the remote. - // (Refer to https://github.com/yorkie-team/yorkie/issues/663) reverseOp = SetOperation.create( this.key, value.deepcopy(), diff --git a/test/integration/document_test.ts b/test/integration/document_test.ts index b885c8940..066c1679c 100644 --- a/test/integration/document_test.ts +++ b/test/integration/document_test.ts @@ -158,9 +158,6 @@ describe('Document', function () { expectedEventValue = [ { type: 'set', path: '$', key: 'counter' }, { type: 'set', path: '$', key: 'todos' }, - { type: 'add', path: '$.todos', index: 0 }, - { type: 'add', path: '$.todos', index: 1 }, - { type: 'add', path: '$.todos', index: 2 }, { type: 'set', path: '$', key: 'content' }, { type: 'edit', @@ -173,16 +170,7 @@ describe('Document', function () { path: '$.content', }, { type: 'set', path: '$', key: 'obj' }, - { type: 'set', path: '$.obj', key: 'name' }, - { type: 'set', path: '$.obj', key: 'age' }, - { type: 'set', path: '$.obj', key: 'food' }, - { type: 'add', path: '$.obj.food', index: 0 }, - { type: 'add', path: '$.obj.food', index: 1 }, - { type: 'set', path: '$.obj', key: 'score' }, - { type: 'set', path: '$.obj.score', key: 'english' }, - { type: 'set', path: '$.obj.score', key: 'math' }, { type: 'set', path: '$.obj', key: 'score' }, - { type: 'set', path: '$.obj.score', key: 'science' }, { type: 'remove', path: '$.obj', key: 'food' }, ]; await eventCollectorD1.waitAndVerifyNthEvent(1, { @@ -277,12 +265,6 @@ describe('Document', function () { await eventCollector.waitAndVerifyNthEvent(1, [ { type: 'set', path: '$', key: 'counter' }, { type: 'set', path: '$', key: 'todos' }, - { type: 'add', path: '$.todos', index: 0 }, - { type: 'add', path: '$.todos', index: 1 }, - ]); - await eventCollectorForTodos.waitAndVerifyNthEvent(1, [ - { type: 'add', path: '$.todos', index: 0 }, - { type: 'add', path: '$.todos', index: 1 }, ]); d2.update((root) => { @@ -301,7 +283,7 @@ describe('Document', function () { await eventCollector.waitAndVerifyNthEvent(3, [ { type: 'add', path: '$.todos', index: 2 }, ]); - await eventCollectorForTodos.waitAndVerifyNthEvent(2, [ + await eventCollectorForTodos.waitAndVerifyNthEvent(1, [ { type: 'add', path: '$.todos', index: 2 }, ]); @@ -312,7 +294,7 @@ describe('Document', function () { await eventCollector.waitAndVerifyNthEvent(4, [ { type: 'add', path: '$.todos', index: 3 }, ]); - assert.equal(eventCollectorForTodos.getLength(), 2); // No events after unsubscribing `$.todos` + assert.equal(eventCollectorForTodos.getLength(), 1); // No events after unsubscribing `$.todos` unsubCounter(); d2.update((root) => { @@ -374,21 +356,7 @@ describe('Document', function () { }); await eventCollector.waitAndVerifyNthEvent(1, [ { type: 'set', path: '$', key: 'todos' }, - { type: 'add', path: '$.todos', index: 0 }, - { type: 'set', path: '$.todos.0', key: 'text' }, - { type: 'set', path: '$.todos.0', key: 'completed' }, { type: 'set', path: '$', key: 'obj' }, - { type: 'set', path: '$.obj', key: 'c1' }, - { type: 'set', path: '$.obj.c1', key: 'name' }, - { type: 'set', path: '$.obj.c1', key: 'age' }, - ]); - await eventCollectorForTodos0.waitAndVerifyNthEvent(1, [ - { type: 'set', path: '$.todos.0', key: 'text' }, - { type: 'set', path: '$.todos.0', key: 'completed' }, - ]); - await eventCollectorForObjC1.waitAndVerifyNthEvent(1, [ - { type: 'set', path: '$.obj.c1', key: 'name' }, - { type: 'set', path: '$.obj.c1', key: 'age' }, ]); d2.update((root) => { @@ -397,7 +365,7 @@ describe('Document', function () { await eventCollector.waitAndVerifyNthEvent(2, [ { type: 'set', path: '$.obj.c1', key: 'name' }, ]); - await eventCollectorForObjC1.waitAndVerifyNthEvent(2, [ + await eventCollectorForObjC1.waitAndVerifyNthEvent(1, [ { type: 'set', path: '$.obj.c1', key: 'name' }, ]); @@ -407,7 +375,7 @@ describe('Document', function () { await eventCollector.waitAndVerifyNthEvent(3, [ { type: 'set', path: '$.todos.0', key: 'completed' }, ]); - await eventCollectorForTodos0.waitAndVerifyNthEvent(2, [ + await eventCollectorForTodos0.waitAndVerifyNthEvent(1, [ { type: 'set', path: '$.todos.0', key: 'completed' }, ]); @@ -418,7 +386,7 @@ describe('Document', function () { await eventCollector.waitAndVerifyNthEvent(4, [ { type: 'set', path: '$.todos.0', key: 'text' }, ]); - assert.equal(eventCollectorForTodos0.getLength(), 2); // No events after unsubscribing `$.todos.0` + assert.equal(eventCollectorForTodos0.getLength(), 1); // No events after unsubscribing `$.todos.0` unsubObj(); d2.update((root) => { @@ -427,7 +395,7 @@ describe('Document', function () { await eventCollector.waitAndVerifyNthEvent(5, [ { type: 'set', path: '$.obj.c1', key: 'age' }, ]); - assert.equal(eventCollectorForObjC1.getLength(), 2); // No events after unsubscribing `$.obj.c1` + assert.equal(eventCollectorForObjC1.getLength(), 1); // No events after unsubscribing `$.obj.c1` unsub(); await c1.detach(d1); diff --git a/test/integration/object_test.ts b/test/integration/object_test.ts index 666f61658..37c313781 100644 --- a/test/integration/object_test.ts +++ b/test/integration/object_test.ts @@ -246,14 +246,14 @@ describe('Object', function () { assert.equal(doc.toSortedJSON(), '{"shape":{"color":"black"}}'); assert.deepEqual( doc.getUndoStackForTest().at(-1)?.map(toStringHistoryOp), - ['1:00:1.REMOVE.1:00:2', '0:00:0.REMOVE.1:00:1'], + ['0:00:0.REMOVE.1:00:1'], ); doc.history.undo(); assert.equal(doc.toSortedJSON(), `{}`); assert.deepEqual( doc.getRedoStackForTest().at(-1)?.map(toStringHistoryOp), - ['0:00:0.SET.shape={"color":"black"}', '1:00:1.SET.color="black"'], + ['0:00:0.SET.shape={"color":"black"}'], ); doc.history.redo(); @@ -363,7 +363,7 @@ describe('Object', function () { assertUndoRedo(doc, states); }); - it.skip(`Should ensure convergence of peer's document after undoing nested objects`, async function ({ + it(`Should ensure convergence of peer's document after undoing nested objects`, async function ({ task, }) { // Test scenario: @@ -404,8 +404,7 @@ describe('Object', function () { assert.equal(doc1.toSortedJSON(), '{"shape":{"point":{"x":0,"y":0}}}'); await client1.sync(); await client2.sync(); - // TODO(chacha912): fix test - assert.equal(doc2.toSortedJSON(), '{"shape":{"point":{"x":0,"y":0}}}'); // as-is: {"shape":{"point":{}}} + assert.equal(doc2.toSortedJSON(), '{"shape":{"point":{"x":0,"y":0}}}'); }); it(`Should handle reverse (set) operation targeting elements deleted by other peers`, async function ({ From 2e5192cca42794ba5829d0cd9b2b5f88ce619f8a Mon Sep 17 00:00:00 2001 From: Yourim Cha Date: Mon, 20 Nov 2023 13:55:39 +0900 Subject: [PATCH 2/9] Extract buildCRDTElement logic for constructing CRDTElement --- src/document/json/array.ts | 89 ++++---------- src/document/json/element.ts | 46 ++++++++ src/document/json/object.ts | 149 +++++------------------- src/document/operation/add_operation.ts | 12 +- test/integration/gc_test.ts | 31 ++++- 5 files changed, 134 insertions(+), 193 deletions(-) diff --git a/src/document/json/array.ts b/src/document/json/array.ts index 8b30e364c..e3d1f71bd 100644 --- a/src/document/json/array.ts +++ b/src/document/json/array.ts @@ -24,18 +24,14 @@ import { CRDTContainer, CRDTElement, } from '@yorkie-js-sdk/src/document/crdt/element'; -import { CRDTObject } from '@yorkie-js-sdk/src/document/crdt/object'; import { CRDTArray } from '@yorkie-js-sdk/src/document/crdt/array'; -import { - Primitive, - PrimitiveValue, -} from '@yorkie-js-sdk/src/document/crdt/primitive'; -import { ObjectProxy } from '@yorkie-js-sdk/src/document/json/object'; +import { Primitive } from '@yorkie-js-sdk/src/document/crdt/primitive'; import { JSONElement, WrappedElement, toWrappedElement, toJSONElement, + buildCRDTElement, } from '@yorkie-js-sdk/src/document/json/element'; /** @@ -341,17 +337,8 @@ export class ArrayProxy { ): Array { const elementArray: Array = []; for (const v of value) { - const ticket = context.issueTimeTicket(); - let elem: CRDTElement; - if (Primitive.isSupport(v)) { - elem = Primitive.of(v as PrimitiveValue, ticket); - } else if (Array.isArray(v)) { - elem = CRDTArray.create(ticket, ArrayProxy.buildArray(context, v)); - } else if (typeof v === 'object') { - elem = CRDTObject.create(ticket, ObjectProxy.buildObject(context, v!)); - } else { - throw new TypeError(`Unsupported type of value: ${typeof value}`); - } + const createdAt = context.issueTimeTicket(); + const elem = buildCRDTElement(context, v, createdAt); elementArray.push(elem); } return elementArray; @@ -467,57 +454,25 @@ export class ArrayProxy { prevCreatedAt: TimeTicket, value: unknown, ): CRDTElement { - const ticket = context.issueTimeTicket(); - if (Primitive.isSupport(value)) { - const primitive = Primitive.of(value as PrimitiveValue, ticket); - const clone = primitive.deepcopy(); - target.insertAfter(prevCreatedAt, clone); - context.registerElement(clone, target); - context.push( - AddOperation.create( - target.getCreatedAt(), - prevCreatedAt, - primitive.deepcopy(), - ticket, - ), - ); - return primitive; - } else if (Array.isArray(value)) { - const array = CRDTArray.create( - ticket, - ArrayProxy.buildArray(context, value), - ); - const clone = array.deepcopy(); - target.insertAfter(prevCreatedAt, clone); - context.registerElement(clone, target); - context.push( - AddOperation.create( - target.getCreatedAt(), - prevCreatedAt, - array.deepcopy(), - ticket, - ), - ); - return array; - } else if (typeof value === 'object') { - const obj = CRDTObject.create( - ticket, - ObjectProxy.buildObject(context, value!), - ); - target.insertAfter(prevCreatedAt, obj); - context.registerElement(obj, target); - context.push( - AddOperation.create( - target.getCreatedAt(), - prevCreatedAt, - obj.deepcopy(), - ticket, - ), - ); - return obj; + const createdAt = context.issueTimeTicket(); + const element = buildCRDTElement(context, value, createdAt); + target.insertAfter(prevCreatedAt, element); + context.registerElement(element, target); + if (element instanceof CRDTContainer) { + element.getDescendants((elem, parent) => { + context.registerElement(elem, parent); + return false; + }); } - - throw new TypeError(`Unsupported type of value: ${typeof value}`); + context.push( + AddOperation.create( + target.getCreatedAt(), + prevCreatedAt, + element.deepcopy(), + createdAt, + ), + ); + return element; } /** diff --git a/src/document/json/element.ts b/src/document/json/element.ts index d7c63d8b4..3c28bb71a 100644 --- a/src/document/json/element.ts +++ b/src/document/json/element.ts @@ -28,14 +28,18 @@ import { CRDTCounter, } from '@yorkie-js-sdk/src/document/crdt/counter'; import { CRDTTree } from '@yorkie-js-sdk/src/document/crdt/tree'; +import { RGATreeSplit } from '@yorkie-js-sdk/src/document/crdt/rga_tree_split'; +import { TimeTicket } from '@yorkie-js-sdk/src/document/time/ticket'; import { JSONObject, createJSONObject, + ObjectProxy, } from '@yorkie-js-sdk/src/document/json/object'; import { JSONArray, createJSONArray, + ArrayProxy, } from '@yorkie-js-sdk/src/document/json/array'; import { Text } from '@yorkie-js-sdk/src/document/json/text'; import { Counter } from '@yorkie-js-sdk/src/document/json/counter'; @@ -131,3 +135,45 @@ export function toJSONElement( return wrappedElement; } + +/** + * `buildCRDTElement` constructs a CRDTElement from the given value. + */ +export function buildCRDTElement( + context: ChangeContext, + value: unknown, + createdAt: TimeTicket, +): CRDTElement { + let element: CRDTElement; + if (Primitive.isSupport(value)) { + element = Primitive.of(value as PrimitiveValue, createdAt); + } else if (Array.isArray(value)) { + element = CRDTArray.create( + createdAt, + ArrayProxy.buildArray(context, value), + ); + } else if (typeof value === 'object') { + if (value instanceof Text) { + element = CRDTText.create(RGATreeSplit.create(), createdAt); + value.initialize(context, element as CRDTText); + } else if (value instanceof Counter) { + element = CRDTCounter.create( + value.getValueType(), + value.getValue(), + createdAt, + ); + value.initialize(context, element as CRDTCounter); + } else if (value instanceof Tree) { + element = CRDTTree.create(value.buildRoot(context), createdAt); + value.initialize(context, element as CRDTTree); + } else { + element = CRDTObject.create( + createdAt, + ObjectProxy.buildObject(context, value!), + ); + } + } else { + throw new TypeError(`Unsupported type of value: ${typeof value}`); + } + return element; +} diff --git a/src/document/json/object.ts b/src/document/json/object.ts index 603d5fff0..798cb5e8a 100644 --- a/src/document/json/object.ts +++ b/src/document/json/object.ts @@ -20,22 +20,15 @@ import { TimeTicket } from '@yorkie-js-sdk/src/document/time/ticket'; import { SetOperation } from '@yorkie-js-sdk/src/document/operation/set_operation'; import { RemoveOperation } from '@yorkie-js-sdk/src/document/operation/remove_operation'; import { ChangeContext } from '@yorkie-js-sdk/src/document/change/context'; -import { CRDTElement } from '@yorkie-js-sdk/src/document/crdt/element'; +import { + CRDTContainer, + CRDTElement, +} from '@yorkie-js-sdk/src/document/crdt/element'; import { CRDTObject } from '@yorkie-js-sdk/src/document/crdt/object'; -import { CRDTArray } from '@yorkie-js-sdk/src/document/crdt/array'; import { - Primitive, - PrimitiveValue, -} from '@yorkie-js-sdk/src/document/crdt/primitive'; -import { RGATreeSplit } from '@yorkie-js-sdk/src/document/crdt/rga_tree_split'; -import { CRDTText } from '@yorkie-js-sdk/src/document/crdt/text'; -import { ArrayProxy } from '@yorkie-js-sdk/src/document/json/array'; -import { Text } from '@yorkie-js-sdk/src/document/json/text'; -import { toJSONElement } from '@yorkie-js-sdk/src/document/json/element'; -import { CRDTCounter } from '@yorkie-js-sdk/src/document/crdt/counter'; -import { Counter } from '@yorkie-js-sdk/src/document/json/counter'; -import { CRDTTree } from '@yorkie-js-sdk/src/document/crdt/tree'; -import { Tree } from '@yorkie-js-sdk/src/document/json/tree'; + toJSONElement, + buildCRDTElement, +} from '@yorkie-js-sdk/src/document/json/element'; /** * `JSONObject` represents a JSON object, but unlike regular JSON, it has time @@ -155,106 +148,27 @@ export class ObjectProxy { ); } - const ticket = context.issueTimeTicket(); - - const setAndRegister = function (elem: CRDTElement) { - const removed = target.set(key, elem, ticket); - context.registerElement(elem, target); - if (removed) { - context.registerRemovedElement(removed); - } - }; - - if (Primitive.isSupport(value)) { - const primitive = Primitive.of(value as PrimitiveValue, ticket); - setAndRegister(primitive); - context.push( - SetOperation.create(key, primitive, target.getCreatedAt(), ticket), - ); - } else if (Array.isArray(value)) { - const array = CRDTArray.create( - ticket, - ArrayProxy.buildArray(context, value), - ); - array.getDescendants((elem, parent) => { + const createdAt = context.issueTimeTicket(); + const element = buildCRDTElement(context, value, createdAt); + const removed = target.set(key, element, createdAt); + context.registerElement(element, target); + if (removed) { + context.registerRemovedElement(removed); + } + if (element instanceof CRDTContainer) { + element.getDescendants((elem, parent) => { context.registerElement(elem, parent); return false; }); - setAndRegister(array); - context.push( - SetOperation.create( - key, - array.deepcopy(), - target.getCreatedAt(), - ticket, - ), - ); - } else if (typeof value === 'object') { - if (value instanceof Text) { - const text = CRDTText.create(RGATreeSplit.create(), ticket); - target.set(key, text, ticket); - context.registerElement(text, target); - context.push( - SetOperation.create( - key, - text.deepcopy(), - target.getCreatedAt(), - ticket, - ), - ); - value.initialize(context, text); - } else if (value instanceof Counter) { - const counter = CRDTCounter.create( - value.getValueType(), - value.getValue(), - ticket, - ); - target.set(key, counter, ticket); - context.registerElement(counter, target); - context.push( - SetOperation.create( - key, - counter.deepcopy(), - target.getCreatedAt(), - ticket, - ), - ); - value.initialize(context, counter); - } else if (value instanceof Tree) { - const tree = CRDTTree.create(value.buildRoot(context), ticket); - target.set(key, tree, ticket); - context.registerElement(tree, target); - context.push( - SetOperation.create( - key, - tree.deepcopy(), - target.getCreatedAt(), - ticket, - ), - ); - value.initialize(context, tree); - } else { - const obj = CRDTObject.create( - ticket, - ObjectProxy.buildObject(context, value!), - ); - obj.getDescendants((elem, parent) => { - context.registerElement(elem, parent); - return false; - }); - setAndRegister(obj); - context.push( - SetOperation.create( - key, - obj.deepcopy(), - target.getCreatedAt(), - ticket, - ), - ); - } - } else { - logger.fatal(`unsupported type of value: ${typeof value}`); } + context.push( + SetOperation.create( + key, + element.deepcopy(), + target.getCreatedAt(), + createdAt, + ), + ); } /** @@ -276,19 +190,8 @@ export class ObjectProxy { ); } - const ticket = context.issueTimeTicket(); - let elem: CRDTElement; - if (Primitive.isSupport(v)) { - elem = Primitive.of(v as PrimitiveValue, ticket); - } else if (Array.isArray(v)) { - const array = ArrayProxy.buildArray(context, v); - elem = CRDTArray.create(ticket, array); - } else if (typeof v === 'object') { - const object = ObjectProxy.buildObject(context, v); - elem = CRDTObject.create(ticket, object); - } else { - throw new TypeError(`Unsupported type of value: ${typeof value}`); - } + const createdAt = context.issueTimeTicket(); + const elem = buildCRDTElement(context, v, createdAt); elementObject[k] = elem; } return elementObject; diff --git a/src/document/operation/add_operation.ts b/src/document/operation/add_operation.ts index 6412f380b..efa0860f3 100644 --- a/src/document/operation/add_operation.ts +++ b/src/document/operation/add_operation.ts @@ -16,7 +16,10 @@ import { logger } from '@yorkie-js-sdk/src/util/logger'; import { TimeTicket } from '@yorkie-js-sdk/src/document/time/ticket'; -import { CRDTElement } from '@yorkie-js-sdk/src/document/crdt/element'; +import { + CRDTContainer, + CRDTElement, +} from '@yorkie-js-sdk/src/document/crdt/element'; import { CRDTRoot } from '@yorkie-js-sdk/src/document/crdt/root'; import { CRDTArray } from '@yorkie-js-sdk/src/document/crdt/array'; import { @@ -69,6 +72,13 @@ export class AddOperation extends Operation { const value = this.value.deepcopy(); array.insertAfter(this.prevCreatedAt, value); root.registerElement(value, array); + if (value instanceof CRDTContainer) { + value.getDescendants((elem, parent) => { + root.registerElement(elem, parent); + return false; + }); + } + return { opInfos: [ { diff --git a/test/integration/gc_test.ts b/test/integration/gc_test.ts index 358a34bf6..48b17052a 100644 --- a/test/integration/gc_test.ts +++ b/test/integration/gc_test.ts @@ -1,12 +1,11 @@ import { describe, it, assert } from 'vitest'; import { MaxTimeTicket } from '@yorkie-js-sdk/src/document/time/ticket'; import { CRDTArray } from '@yorkie-js-sdk/src/document/crdt/array'; -import yorkie, { Tree } from '@yorkie-js-sdk/src/yorkie'; +import yorkie, { Text, Tree } from '@yorkie-js-sdk/src/yorkie'; import { testRPCAddr, toDocKey, } from '@yorkie-js-sdk/test/integration/integration_helper'; -import { Text } from '@yorkie-js-sdk/src/yorkie'; import { CRDTTreeNode } from '@yorkie-js-sdk/src/document/crdt/tree'; import { IndexTreeNode } from '@yorkie-js-sdk/src/util/index_tree'; @@ -610,6 +609,34 @@ describe('Garbage Collection', function () { assert.equal(doc.getGarbageLenFromClone(), 6); }); + it('Can collect removed elements from both root and clone for nested array', async function ({ + task, + }) { + type TestDoc = { list: Array> }; + const docKey = toDocKey(`${task.name}-${new Date().getTime()}`); + const doc = new yorkie.Document(docKey); + const cli = new yorkie.Client(testRPCAddr); + await cli.activate(); + + await cli.attach(doc, { isRealtimeSync: false }); + doc.update((root) => { + root.list = [0, 1, 2]; + root.list.push([3, 4, 5]); + }); + assert.equal('{"list":[0,1,2,[3,4,5]]}', doc.toJSON()); + doc.update((root) => { + delete root.list[1]; + }); + assert.equal('{"list":[0,2,[3,4,5]]}', doc.toJSON()); + doc.update((root) => { + delete (root.list[2] as Array)[1]; + }); + assert.equal('{"list":[0,2,[3,5]]}', doc.toJSON()); + + assert.equal(doc.getGarbageLen(), 2); + assert.equal(doc.getGarbageLenFromClone(), 2); + }); + it('Can purges removed elements after peers can not access them', async function ({ task, }) { From 4058e5b2af02faf12778facd528ee2bf41a9c33b Mon Sep 17 00:00:00 2001 From: Yourim Cha Date: Mon, 20 Nov 2023 16:20:25 +0900 Subject: [PATCH 3/9] Apply to whiteboard example --- public/whiteboard.html | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/public/whiteboard.html b/public/whiteboard.html index ba70d3b63..baaee3c89 100644 --- a/public/whiteboard.html +++ b/public/whiteboard.html @@ -169,8 +169,7 @@

const selectedShape = root.shapes.find( (shape) => shape.id === selectedShapeID, ); - selectedShape.point.x = movingShapePoint.x; - selectedShape.point.y = movingShapePoint.y; + selectedShape.point = movingShapePoint; presence.set({ movingShapePoint: null }); }); } From 38dc37a576b6868e6e149518a50f01ff62b38789 Mon Sep 17 00:00:00 2001 From: Yourim Cha <81357083+chacha912@users.noreply.github.com> Date: Wed, 22 Nov 2023 11:21:16 +0900 Subject: [PATCH 4/9] Prevent empty ops are applied during undo/redo (#687) This commit addresses the issue of client sequences being absent on the server. It accomplishes this by preventing the update of changeID. --- src/document/document.ts | 29 +++++++------ test/integration/object_test.ts | 76 +++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 12 deletions(-) diff --git a/src/document/document.ts b/src/document/document.ts index 097acc8ca..0dbc4d441 100644 --- a/src/document/document.ts +++ b/src/document/document.ts @@ -550,6 +550,7 @@ export class Document { this.changeID = change.getID(); // 03. Publish the document change event. + // NOTE(chacha912): Check opInfos, which represent the actually executed operations. if (opInfos.length > 0) { this.publish({ type: DocEventType.LocalChange, @@ -846,6 +847,15 @@ export class Document { return this.checkpoint; } + /** + * `getChangeID` returns the change id of this document. + * + * @internal + */ + public getChangeID(): ChangeID { + return this.changeID; + } + /** * `hasLocalChanges` returns whether this document has local changes or not. * @@ -1309,18 +1319,15 @@ export class Document { this.internalHistory.pushRedo(reverseOps); } - // TODO(chacha912): When there is no applied operation or presence + // NOTE(chacha912): When there is no applied operation or presence // during undo/redo, skip propagating change remotely. - // In the database, it may appear as if the client sequence is missing. - if (change.hasPresenceChange() || opInfos.length > 0) { - this.localChanges.push(change); + if (!change.hasPresenceChange() && opInfos.length === 0) { + return; } + this.localChanges.push(change); this.changeID = change.getID(); const actorID = this.changeID.getActorID()!; - // NOTE(chacha912): Although operations are included in the change, they - // may not be executable (e.g., when the target element has been deleted). - // So we check opInfos, which represent the actually executed operations. if (opInfos.length > 0) { this.publish({ type: DocEventType.LocalChange, @@ -1400,15 +1407,13 @@ export class Document { // NOTE(chacha912): When there is no applied operation or presence // during undo/redo, skip propagating change remotely. - if (change.hasPresenceChange() || opInfos.length > 0) { - this.localChanges.push(change); + if (!change.hasPresenceChange() && opInfos.length === 0) { + return; } + this.localChanges.push(change); this.changeID = change.getID(); const actorID = this.changeID.getActorID()!; - // NOTE(chacha912): Although operations are included in the change, they - // may not be executable (e.g., when the target element has been deleted). - // So we check opInfos, which represent the actually executed operations. if (opInfos.length > 0) { this.publish({ type: DocEventType.LocalChange, diff --git a/test/integration/object_test.ts b/test/integration/object_test.ts index 666f61658..8a693795b 100644 --- a/test/integration/object_test.ts +++ b/test/integration/object_test.ts @@ -507,6 +507,82 @@ describe('Object', function () { await client1.sync(); }); + it(`Should not propagate changes when there is no applied undo operation`, async function ({ + task, + }) { + interface TestDoc { + shape?: { color: string }; + } + const docKey = toDocKey(`${task.name}-${new Date().getTime()}`); + const doc1 = new Document(docKey); + const doc2 = new Document(docKey); + + const client1 = new Client(testRPCAddr); + const client2 = new Client(testRPCAddr); + await client1.activate(); + await client2.activate(); + + await client1.attach(doc1, { isRealtimeSync: false }); + let doc1ChangeID = doc1.getChangeID(); + let doc1Checkpoint = doc1.getCheckpoint(); + assert.equal(doc1ChangeID.getClientSeq(), 1); + assert.equal(doc1Checkpoint.getClientSeq(), 1); + assert.equal(doc1Checkpoint.getServerSeq().toInt(), 1); + + doc1.update((root) => { + root.shape = { color: 'black' }; + }, 'init doc'); + await client1.sync(); + assert.equal(doc1.toSortedJSON(), '{"shape":{"color":"black"}}'); + doc1ChangeID = doc1.getChangeID(); + doc1Checkpoint = doc1.getCheckpoint(); + assert.equal(doc1ChangeID.getClientSeq(), 2); + assert.equal(doc1Checkpoint.getClientSeq(), 2); + assert.equal(doc1Checkpoint.getServerSeq().toInt(), 2); + + await client2.attach(doc2, { isRealtimeSync: false }); + assert.equal(doc2.toSortedJSON(), '{"shape":{"color":"black"}}'); + + doc2.update((root) => { + delete root.shape; + }, 'delete shape'); + await client2.sync(); + await client1.sync(); + assert.equal(doc1.toSortedJSON(), '{}'); + assert.equal(doc2.toSortedJSON(), '{}'); + doc1ChangeID = doc1.getChangeID(); + doc1Checkpoint = doc1.getCheckpoint(); + assert.equal(doc1ChangeID.getClientSeq(), 2); + assert.equal(doc1Checkpoint.getClientSeq(), 2); + assert.equal(doc1Checkpoint.getServerSeq().toInt(), 4); + + // c2 deleted the shape, so the reverse operation cannot be applied + doc1.history.undo(); + assert.equal(doc1.toSortedJSON(), '{}'); + assert.equal(doc1.getRedoStackForTest().length, 0); + assert.equal(doc1.history.canRedo(), false); + await client1.sync(); + await client2.sync(); + await client1.sync(); + // Since there are no applied operations, there should be no change in the sequence. + doc1ChangeID = doc1.getChangeID(); + doc1Checkpoint = doc1.getCheckpoint(); + assert.equal(doc1ChangeID.getClientSeq(), 2); + assert.equal(doc1Checkpoint.getClientSeq(), 2); + assert.equal(doc1Checkpoint.getServerSeq().toInt(), 4); + + doc1.update((root) => { + root.shape = { color: 'red' }; + }); + await client1.sync(); + assert.equal(doc1.toSortedJSON(), '{"shape":{"color":"red"}}'); + doc1ChangeID = doc1.getChangeID(); + doc1Checkpoint = doc1.getCheckpoint(); + assert.equal(doc1ChangeID.getClientSeq(), 3); + assert.equal(doc1Checkpoint.getClientSeq(), 3); + assert.equal(doc1Checkpoint.getServerSeq().toInt(), 5); + }); + it('Can handle concurrent undo/redo: local undo & global redo', async function ({ task, }) { From b4b65047a02ce64f4dd28e7b70d7f7844e34ad80 Mon Sep 17 00:00:00 2001 From: Yourim Cha Date: Wed, 22 Nov 2023 12:26:27 +0900 Subject: [PATCH 5/9] Handle reverse operation for nest object that other peers deleted --- src/document/crdt/root.ts | 10 ++ src/document/operation/remove_operation.ts | 18 ++- src/document/operation/set_operation.ts | 10 +- test/integration/object_test.ts | 134 ++++++++++++++++++++- 4 files changed, 162 insertions(+), 10 deletions(-) diff --git a/src/document/crdt/root.ts b/src/document/crdt/root.ts index 5e0c89596..c15bfdbad 100644 --- a/src/document/crdt/root.ts +++ b/src/document/crdt/root.ts @@ -115,6 +115,16 @@ export class CRDTRoot { return pair.element; } + /** + * `findElementPairByCreatedAt` returns the element and parent pair + * of given creation time. + */ + public findElementPairByCreatedAt( + createdAt: TimeTicket, + ): CRDTElementPair | undefined { + return this.elementPairMapByCreatedAt.get(createdAt.toIDString()); + } + /** * `createSubPaths` creates an array of the sub paths for the given element. */ diff --git a/src/document/operation/remove_operation.ts b/src/document/operation/remove_operation.ts index 49c41bee3..666d24f5f 100644 --- a/src/document/operation/remove_operation.ts +++ b/src/document/operation/remove_operation.ts @@ -72,12 +72,18 @@ export class RemoveOperation extends Operation { } // NOTE(chacha912): Handle cases where operation cannot be executed during undo and redo. - const targetElem = parentObject.getByID(this.createdAt); - if ( - source === OpSource.UndoRedo && - (parentObject.getRemovedAt() || !targetElem || targetElem.isRemoved()) - ) { - return; + if (source === OpSource.UndoRedo) { + const targetElem = parentObject.getByID(this.createdAt); + if (targetElem?.isRemoved()) { + return; + } + let parent: CRDTContainer | undefined = parentObject; + while (parent) { + if (parent.getRemovedAt()) { + return; + } + parent = root.findElementPairByCreatedAt(parent.getCreatedAt())?.parent; + } } const key = parentObject.subPathOf(this.createdAt); const reverseOp = this.toReverseOperation(parentObject); diff --git a/src/document/operation/set_operation.ts b/src/document/operation/set_operation.ts index 80ad93dd4..b4ca72162 100644 --- a/src/document/operation/set_operation.ts +++ b/src/document/operation/set_operation.ts @@ -76,8 +76,14 @@ export class SetOperation extends Operation { } const obj = parentObject as CRDTObject; // NOTE(chacha912): Handle cases where operation cannot be executed during undo and redo. - if (source === OpSource.UndoRedo && obj.getRemovedAt()) { - return; + if (source === OpSource.UndoRedo) { + let parent: CRDTContainer | undefined = obj; + while (parent) { + if (parent.getRemovedAt()) { + return; + } + parent = root.findElementPairByCreatedAt(parent.getCreatedAt())?.parent; + } } const previousValue = obj.get(this.key); const reverseOp = this.toReverseOperation(previousValue); diff --git a/test/integration/object_test.ts b/test/integration/object_test.ts index 98f6157b5..61ba77e9f 100644 --- a/test/integration/object_test.ts +++ b/test/integration/object_test.ts @@ -407,7 +407,7 @@ describe('Object', function () { assert.equal(doc2.toSortedJSON(), '{"shape":{"point":{"x":0,"y":0}}}'); }); - it(`Should handle reverse (set) operation targeting elements deleted by other peers`, async function ({ + it(`Should handle reverse set operation for elements that other peers deleted`, async function ({ task, }) { // Test scenario: @@ -454,12 +454,80 @@ describe('Object', function () { assert.equal(doc1.toSortedJSON(), '{}'); assert.equal(doc1.getRedoStackForTest().length, 0); assert.equal(doc1.history.canRedo(), false); + }); + + it(`Should handle reverse set operation for elements (nested objects) that other peers deleted`, async function ({ + task, + }) { + // Test scenario: + // c1: create shape + // c1: set shape.circle.point to { x: 1, y: 1 } + // c2: delete shape + // c1: undo(no changes as the shape was deleted) + interface TestDoc { + shape?: { circle: { point: { x: number; y: number } } }; + } + const docKey = toDocKey(`${task.name}-${new Date().getTime()}`); + const doc1 = new Document(docKey); + const doc2 = new Document(docKey); + + const client1 = new Client(testRPCAddr); + const client2 = new Client(testRPCAddr); + await client1.activate(); + await client2.activate(); + + await client1.attach(doc1, { isRealtimeSync: false }); + doc1.update((root) => { + root.shape = { circle: { point: { x: 0, y: 0 } } }; + }); await client1.sync(); + assert.equal( + doc1.toSortedJSON(), + '{"shape":{"circle":{"point":{"x":0,"y":0}}}}', + ); + + await client2.attach(doc2, { isRealtimeSync: false }); + assert.equal( + doc2.toSortedJSON(), + '{"shape":{"circle":{"point":{"x":0,"y":0}}}}', + ); + + doc1.update((root) => { + root.shape!.circle.point = { x: 1, y: 1 }; + }); + await client1.sync(); + await client2.sync(); + assert.equal( + doc1.toSortedJSON(), + '{"shape":{"circle":{"point":{"x":1,"y":1}}}}', + ); + assert.equal( + doc2.toSortedJSON(), + '{"shape":{"circle":{"point":{"x":1,"y":1}}}}', + ); + doc2.update((root) => { + delete root.shape; + }, 'delete shape'); await client2.sync(); await client1.sync(); + assert.equal(doc1.toSortedJSON(), '{}'); + assert.equal(doc2.toSortedJSON(), '{}'); + + const c1ID = client1.getID()!.slice(-2); + assert.deepEqual( + doc1.getUndoStackForTest().at(-1)?.map(toStringHistoryOp), + [`2:${c1ID}:2.SET.point={"x":0,"y":0}`], + ); + doc1.history.undo(); + assert.equal(doc1.toSortedJSON(), '{}'); + await client1.sync(); + await client2.sync(); + assert.equal(doc2.toSortedJSON(), '{}'); + assert.equal(doc1.getRedoStackForTest().length, 0); + assert.equal(doc1.history.canRedo(), false); }); - it(`Should handle reverse (remove) operation targeting elements deleted by other peers`, async function ({ + it(`Should handle reverse remove operation for elements that other peers deleted`, async function ({ task, }) { // Test scenario: @@ -501,9 +569,71 @@ describe('Object', function () { assert.equal(doc1.toSortedJSON(), '{}'); assert.equal(doc1.getRedoStackForTest().length, 0); assert.equal(doc1.history.canRedo(), false); + }); + + it(`Should handle reverse remove operation for elements (nested objects) that other peers deleted`, async function ({ + task, + }) { + // Test scenario: + // c1: set shape.circle.point to { x: 0, y: 0 } + // c2: delete shape + // c1: undo(no changes as the shape was deleted) + interface TestDoc { + shape?: { + circle?: { point?: { x?: number; y?: number }; color?: string }; + }; + } + const docKey = toDocKey(`${task.name}-${new Date().getTime()}`); + const doc1 = new Document(docKey); + const doc2 = new Document(docKey); + + const client1 = new Client(testRPCAddr); + const client2 = new Client(testRPCAddr); + await client1.activate(); + await client2.activate(); + + await client1.attach(doc1, { isRealtimeSync: false }); + doc1.update((root) => { + root.shape = { circle: { color: 'red' } }; + }); await client1.sync(); + assert.equal(doc1.toSortedJSON(), '{"shape":{"circle":{"color":"red"}}}'); + + await client2.attach(doc2, { isRealtimeSync: false }); + assert.equal(doc2.toSortedJSON(), '{"shape":{"circle":{"color":"red"}}}'); + + doc1.update((root) => { + root.shape!.circle!.point = { x: 0, y: 0 }; + }); + await client1.sync(); + await client2.sync(); + assert.equal( + doc1.toSortedJSON(), + '{"shape":{"circle":{"color":"red","point":{"x":0,"y":0}}}}', + ); + assert.equal( + doc2.toSortedJSON(), + '{"shape":{"circle":{"color":"red","point":{"x":0,"y":0}}}}', + ); + doc2.update((root) => { + delete root.shape; + }, 'delete shape'); await client2.sync(); await client1.sync(); + assert.equal(doc1.toSortedJSON(), '{}'); + assert.equal(doc2.toSortedJSON(), '{}'); + + const c1ID = client1.getID()!.slice(-2); + assert.deepEqual( + doc1.getUndoStackForTest().at(-1)?.map(toStringHistoryOp), + [`2:${c1ID}:2.REMOVE.3:${c1ID}:1`], + ); + doc1.history.undo(); + assert.equal(doc1.toSortedJSON(), '{}'); + await client1.sync(); + await client2.sync(); + assert.equal(doc2.toSortedJSON(), '{}'); + assert.deepEqual(doc1.getRedoStackForTest().length, 0); }); it(`Should not propagate changes when there is no applied undo operation`, async function ({ From 24d7e3b98712e115212efdc367bc8ed0666e62b5 Mon Sep 17 00:00:00 2001 From: Yourim Cha Date: Wed, 22 Nov 2023 18:58:47 +0900 Subject: [PATCH 6/9] Handle tombstoned elements when deleted node is restored through undo --- src/document/crdt/root.ts | 76 ++++++++++++------------- src/document/json/array.ts | 11 +--- src/document/json/object.ts | 11 +--- src/document/operation/add_operation.ts | 11 +--- src/document/operation/set_operation.ts | 15 +++-- test/integration/object_test.ts | 45 +++++++++++++++ 6 files changed, 93 insertions(+), 76 deletions(-) diff --git a/src/document/crdt/root.ts b/src/document/crdt/root.ts index c15bfdbad..9b67a56bc 100644 --- a/src/document/crdt/root.ts +++ b/src/document/crdt/root.ts @@ -82,18 +82,7 @@ export class CRDTRoot { this.removedElementSetByCreatedAt = new Set(); this.elementHasRemovedNodesSetByCreatedAt = new Set(); this.opsForTest = []; - - this.elementPairMapByCreatedAt.set( - this.rootObject.getCreatedAt().toIDString(), - { element: this.rootObject }, - ); - - rootObject.getDescendants( - (elem: CRDTElement, parent: CRDTContainer): boolean => { - this.registerElement(elem, parent); - return false; - }, - ); + this.registerElement(rootObject, undefined); } /** @@ -160,23 +149,49 @@ export class CRDTRoot { } /** - * `registerElement` registers the given element to hash table. + * `registerElement` registers the given element and its descendants to hash table. */ - public registerElement(element: CRDTElement, parent: CRDTContainer): void { + public registerElement(element: CRDTElement, parent?: CRDTContainer): void { this.elementPairMapByCreatedAt.set(element.getCreatedAt().toIDString(), { parent, element, }); + + if (element instanceof CRDTContainer) { + element.getDescendants((elem, parent) => { + this.registerElement(elem, parent); + return false; + }); + } } /** - * `deregisterElement` deregister the given element from hash table. + * `deregisterElement` deregister the given element and its descendants from hash table. */ - public deregisterElement(element: CRDTElement): void { - this.elementPairMapByCreatedAt.delete(element.getCreatedAt().toIDString()); - this.removedElementSetByCreatedAt.delete( - element.getCreatedAt().toIDString(), - ); + public deregisterElement(element: CRDTElement): number { + let count = 0; + + const callback = (elem: CRDTElement) => { + const createdAt = elem.getCreatedAt().toIDString(); + this.elementPairMapByCreatedAt.delete(createdAt); + this.removedElementSetByCreatedAt.delete(createdAt); + count++; + }; + const deregisterDescendants = (container: CRDTContainer) => { + container.getDescendants((elem) => { + callback(elem); + if (elem instanceof CRDTContainer) { + deregisterDescendants(elem); + } + return false; + }); + }; + + callback(element); + if (element instanceof CRDTContainer) { + deregisterDescendants(element); + } + return count; } /** @@ -263,7 +278,7 @@ export class CRDTRoot { ticket.compare(pair.element.getRemovedAt()!) >= 0 ) { pair.parent!.purge(pair.element); - count += this.garbageCollectInternal(pair.element); + count += this.deregisterElement(pair.element); } } @@ -283,25 +298,6 @@ export class CRDTRoot { return count; } - private garbageCollectInternal(element: CRDTElement): number { - let count = 0; - - // eslint-disable-next-line @typescript-eslint/no-unused-vars - const callback = (elem: CRDTElement, parent?: CRDTContainer): boolean => { - this.deregisterElement(elem); - count++; - return false; - }; - - callback(element); - - if (element instanceof CRDTContainer) { - element.getDescendants(callback); - } - - return count; - } - /** * `toJSON` returns the JSON encoding of this root object. */ diff --git a/src/document/json/array.ts b/src/document/json/array.ts index e3d1f71bd..ca26f17f4 100644 --- a/src/document/json/array.ts +++ b/src/document/json/array.ts @@ -20,10 +20,7 @@ import { AddOperation } from '@yorkie-js-sdk/src/document/operation/add_operatio import { MoveOperation } from '@yorkie-js-sdk/src/document/operation/move_operation'; import { RemoveOperation } from '@yorkie-js-sdk/src/document/operation/remove_operation'; import { ChangeContext } from '@yorkie-js-sdk/src/document/change/context'; -import { - CRDTContainer, - CRDTElement, -} from '@yorkie-js-sdk/src/document/crdt/element'; +import { CRDTElement } from '@yorkie-js-sdk/src/document/crdt/element'; import { CRDTArray } from '@yorkie-js-sdk/src/document/crdt/array'; import { Primitive } from '@yorkie-js-sdk/src/document/crdt/primitive'; import { @@ -458,12 +455,6 @@ export class ArrayProxy { const element = buildCRDTElement(context, value, createdAt); target.insertAfter(prevCreatedAt, element); context.registerElement(element, target); - if (element instanceof CRDTContainer) { - element.getDescendants((elem, parent) => { - context.registerElement(elem, parent); - return false; - }); - } context.push( AddOperation.create( target.getCreatedAt(), diff --git a/src/document/json/object.ts b/src/document/json/object.ts index 798cb5e8a..030d6b488 100644 --- a/src/document/json/object.ts +++ b/src/document/json/object.ts @@ -20,10 +20,7 @@ import { TimeTicket } from '@yorkie-js-sdk/src/document/time/ticket'; import { SetOperation } from '@yorkie-js-sdk/src/document/operation/set_operation'; import { RemoveOperation } from '@yorkie-js-sdk/src/document/operation/remove_operation'; import { ChangeContext } from '@yorkie-js-sdk/src/document/change/context'; -import { - CRDTContainer, - CRDTElement, -} from '@yorkie-js-sdk/src/document/crdt/element'; +import { CRDTElement } from '@yorkie-js-sdk/src/document/crdt/element'; import { CRDTObject } from '@yorkie-js-sdk/src/document/crdt/object'; import { toJSONElement, @@ -155,12 +152,6 @@ export class ObjectProxy { if (removed) { context.registerRemovedElement(removed); } - if (element instanceof CRDTContainer) { - element.getDescendants((elem, parent) => { - context.registerElement(elem, parent); - return false; - }); - } context.push( SetOperation.create( key, diff --git a/src/document/operation/add_operation.ts b/src/document/operation/add_operation.ts index efa0860f3..ec7208677 100644 --- a/src/document/operation/add_operation.ts +++ b/src/document/operation/add_operation.ts @@ -16,10 +16,7 @@ import { logger } from '@yorkie-js-sdk/src/util/logger'; import { TimeTicket } from '@yorkie-js-sdk/src/document/time/ticket'; -import { - CRDTContainer, - CRDTElement, -} from '@yorkie-js-sdk/src/document/crdt/element'; +import { CRDTElement } from '@yorkie-js-sdk/src/document/crdt/element'; import { CRDTRoot } from '@yorkie-js-sdk/src/document/crdt/root'; import { CRDTArray } from '@yorkie-js-sdk/src/document/crdt/array'; import { @@ -72,12 +69,6 @@ export class AddOperation extends Operation { const value = this.value.deepcopy(); array.insertAfter(this.prevCreatedAt, value); root.registerElement(value, array); - if (value instanceof CRDTContainer) { - value.getDescendants((elem, parent) => { - root.registerElement(elem, parent); - return false; - }); - } return { opInfos: [ diff --git a/src/document/operation/set_operation.ts b/src/document/operation/set_operation.ts index b4ca72162..0d8d7240d 100644 --- a/src/document/operation/set_operation.ts +++ b/src/document/operation/set_operation.ts @@ -90,16 +90,19 @@ export class SetOperation extends Operation { const value = this.value.deepcopy(); const removed = obj.set(this.key, value, this.getExecutedAt()); + // NOTE(chacha912): When resetting elements with the pre-existing createdAt + // during undo/redo, it's essential to handle previously tombstoned elements. + // In non-GC languages, there may be a need to execute both deregister and purge. + if ( + source === OpSource.UndoRedo && + root.findByCreatedAt(value.getCreatedAt()) + ) { + root.deregisterElement(value); + } root.registerElement(value, obj); if (removed) { root.registerRemovedElement(removed); } - if (value instanceof CRDTContainer) { - value.getDescendants((elem, parent) => { - root.registerElement(elem, parent); - return false; - }); - } return { opInfos: [ diff --git a/test/integration/object_test.ts b/test/integration/object_test.ts index 61ba77e9f..9fd8b31ca 100644 --- a/test/integration/object_test.ts +++ b/test/integration/object_test.ts @@ -868,5 +868,50 @@ describe('Object', function () { await client2.sync(); assert.equal(doc2.toSortedJSON(), '{"shape":{"color":"red"}}'); }); + + it(`Should clean up the references to a previously deleted node when the deleted node is restored through undo`, async function ({ + task, + }) { + interface TestDoc { + shape: { color: string }; + } + const docKey = toDocKey(`${task.name}-${new Date().getTime()}`); + const doc1 = new Document(docKey); + const doc2 = new Document(docKey); + + const client1 = new Client(testRPCAddr); + const client2 = new Client(testRPCAddr); + await client1.activate(); + await client2.activate(); + + await client1.attach(doc1, { isRealtimeSync: false }); + await client2.attach(doc2, { isRealtimeSync: false }); + + doc1.update((root) => { + root.shape = { color: 'black' }; + }); + await client1.sync(); + await client2.sync(); + assert.equal(doc1.toSortedJSON(), '{"shape":{"color":"black"}}'); + assert.equal(doc2.toSortedJSON(), '{"shape":{"color":"black"}}'); + + doc2.update((root) => { + root.shape = { color: 'yellow' }; + }); + await client2.sync(); + await client1.sync(); + assert.equal(doc1.toSortedJSON(), '{"shape":{"color":"yellow"}}'); + assert.equal(doc2.toSortedJSON(), '{"shape":{"color":"yellow"}}'); + + doc2.history.undo(); + await client2.sync(); + await client1.sync(); + assert.equal(doc1.toSortedJSON(), '{"shape":{"color":"black"}}'); + assert.equal(doc2.toSortedJSON(), '{"shape":{"color":"black"}}'); + + // NOTE(chacha912): removedElementSetByCreatedAt should only retain + // the entry for `{shape: {color: 'yellow'}}`. + assert.equal(doc2.getGarbageLen(), 2); + }); }); }); From 657da76719166db7f7a1671fa3900723ed97e8e3 Mon Sep 17 00:00:00 2001 From: Yourim Cha Date: Thu, 23 Nov 2023 10:22:45 +0900 Subject: [PATCH 7/9] Add comments on CRDTContainer removal --- src/document/crdt/element.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/document/crdt/element.ts b/src/document/crdt/element.ts index 7cd93f7e8..5eca00a76 100644 --- a/src/document/crdt/element.ts +++ b/src/document/crdt/element.ts @@ -99,6 +99,9 @@ export abstract class CRDTElement { removedAt.after(this.getPositionedAt()) && (!this.removedAt || removedAt.after(this.removedAt)) ) { + // NOTE(chacha912): If it's a CRDTContainer, removedAt is marked only on + // the top-level element, without marking all descendant elements. This + // enhances the speed of deletion. this.removedAt = removedAt; return true; } From dcfd7960e55339f882cbc1d1438921972d2fb4b4 Mon Sep 17 00:00:00 2001 From: Yourim Cha Date: Thu, 23 Nov 2023 14:21:32 +0900 Subject: [PATCH 8/9] Handle cases where values are not set when converting to PbJSONElementSimple --- src/api/converter.ts | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/api/converter.ts b/src/api/converter.ts index e3b59f56b..b86a777ff 100644 --- a/src/api/converter.ts +++ b/src/api/converter.ts @@ -837,8 +837,18 @@ function fromCounterType(pbValueType: PbValueType): CounterType { function fromElementSimple(pbElementSimple: PbJSONElementSimple): CRDTElement { switch (pbElementSimple.getType()) { case PbValueType.VALUE_TYPE_JSON_OBJECT: + if (!pbElementSimple.getValue()) { + return CRDTObject.create( + fromTimeTicket(pbElementSimple.getCreatedAt())!, + ); + } return bytesToObject(pbElementSimple.getValue_asU8()); case PbValueType.VALUE_TYPE_JSON_ARRAY: + if (!pbElementSimple.getValue()) { + return CRDTArray.create( + fromTimeTicket(pbElementSimple.getCreatedAt())!, + ); + } return bytesToArray(pbElementSimple.getValue_asU8()); case PbValueType.VALUE_TYPE_TEXT: return CRDTText.create( @@ -1345,7 +1355,7 @@ function bytesToSnapshot

( */ function bytesToObject(bytes?: Uint8Array): CRDTObject { if (!bytes) { - return CRDTObject.create(InitialTimeTicket); + throw new Error('bytes is empty'); } const pbElement = PbJSONElement.deserializeBinary(bytes); @@ -1362,7 +1372,11 @@ function objectToBytes(obj: CRDTObject): Uint8Array { /** * `bytesToArray` creates an CRDTArray from the given bytes. */ -function bytesToArray(bytes: Uint8Array): CRDTArray { +function bytesToArray(bytes?: Uint8Array): CRDTArray { + if (!bytes) { + throw new Error('bytes is empty'); + } + const pbElement = PbJSONElement.deserializeBinary(bytes); return fromArray(pbElement.getJsonArray()!); } From 5f6fffeeceaa1eb2d761742b47bb0669da32c9c0 Mon Sep 17 00:00:00 2001 From: Youngteac Hong Date: Thu, 23 Nov 2023 18:42:45 +0900 Subject: [PATCH 9/9] Revise codes --- src/document/crdt/root.ts | 24 ++++++++----------- src/document/json/array.ts | 10 ++++---- src/document/json/element.ts | 4 ++-- src/document/json/object.ts | 10 ++++---- src/document/operation/remove_operation.ts | 27 +++++++++++----------- src/document/operation/set_operation.ts | 15 +++++------- 6 files changed, 41 insertions(+), 49 deletions(-) diff --git a/src/document/crdt/root.ts b/src/document/crdt/root.ts index 9b67a56bc..beecb02f3 100644 --- a/src/document/crdt/root.ts +++ b/src/document/crdt/root.ts @@ -171,26 +171,22 @@ export class CRDTRoot { public deregisterElement(element: CRDTElement): number { let count = 0; - const callback = (elem: CRDTElement) => { + const deregisterElementInternal = (elem: CRDTElement) => { const createdAt = elem.getCreatedAt().toIDString(); this.elementPairMapByCreatedAt.delete(createdAt); this.removedElementSetByCreatedAt.delete(createdAt); count++; - }; - const deregisterDescendants = (container: CRDTContainer) => { - container.getDescendants((elem) => { - callback(elem); - if (elem instanceof CRDTContainer) { - deregisterDescendants(elem); - } - return false; - }); + + if (elem instanceof CRDTContainer) { + elem.getDescendants((e) => { + deregisterElementInternal(e); + return false; + }); + } }; - callback(element); - if (element instanceof CRDTContainer) { - deregisterDescendants(element); - } + deregisterElementInternal(element); + return count; } diff --git a/src/document/json/array.ts b/src/document/json/array.ts index ca26f17f4..7c58d347e 100644 --- a/src/document/json/array.ts +++ b/src/document/json/array.ts @@ -326,19 +326,19 @@ export class ArrayProxy { } /** - * `buildArray` constructs an array of CRDTElements based on the user-provided array. + * `buildArrayElements` constructs array elements based on the user-provided array. */ - public static buildArray( + public static buildArrayElements( context: ChangeContext, value: Array, ): Array { - const elementArray: Array = []; + const elements: Array = []; for (const v of value) { const createdAt = context.issueTimeTicket(); const elem = buildCRDTElement(context, v, createdAt); - elementArray.push(elem); + elements.push(elem); } - return elementArray; + return elements; } /** diff --git a/src/document/json/element.ts b/src/document/json/element.ts index 3c28bb71a..65e679414 100644 --- a/src/document/json/element.ts +++ b/src/document/json/element.ts @@ -150,7 +150,7 @@ export function buildCRDTElement( } else if (Array.isArray(value)) { element = CRDTArray.create( createdAt, - ArrayProxy.buildArray(context, value), + ArrayProxy.buildArrayElements(context, value), ); } else if (typeof value === 'object') { if (value instanceof Text) { @@ -169,7 +169,7 @@ export function buildCRDTElement( } else { element = CRDTObject.create( createdAt, - ObjectProxy.buildObject(context, value!), + ObjectProxy.buildObjectMembers(context, value!), ); } } else { diff --git a/src/document/json/object.ts b/src/document/json/object.ts index 030d6b488..e594d13da 100644 --- a/src/document/json/object.ts +++ b/src/document/json/object.ts @@ -163,16 +163,16 @@ export class ObjectProxy { } /** - * `buildObject` constructs an object where all values from the + * `buildObjectMembers` constructs an object where all values from the * user-provided object are transformed into CRDTElements. * This function takes an object and iterates through its values, * converting each value into a corresponding CRDTElement. */ - public static buildObject( + public static buildObjectMembers( context: ChangeContext, value: object, ): { [key: string]: CRDTElement } { - const elementObject: { [key: string]: CRDTElement } = {}; + const members: { [key: string]: CRDTElement } = {}; for (const [k, v] of Object.entries(value)) { if (k.includes('.')) { throw new YorkieError( @@ -183,9 +183,9 @@ export class ObjectProxy { const createdAt = context.issueTimeTicket(); const elem = buildCRDTElement(context, v, createdAt); - elementObject[k] = elem; + members[k] = elem; } - return elementObject; + return members; } /** diff --git a/src/document/operation/remove_operation.ts b/src/document/operation/remove_operation.ts index 666d24f5f..5e88da410 100644 --- a/src/document/operation/remove_operation.ts +++ b/src/document/operation/remove_operation.ts @@ -23,7 +23,10 @@ import { OperationInfo, ExecutionResult, } from '@yorkie-js-sdk/src/document/operation/operation'; -import { CRDTContainer } from '@yorkie-js-sdk/src/document/crdt/element'; +import { + CRDTContainer, + CRDTElement, +} from '@yorkie-js-sdk/src/document/crdt/element'; import { CRDTObject } from '@yorkie-js-sdk/src/document/crdt/object'; import { CRDTArray } from '@yorkie-js-sdk/src/document/crdt/array'; import { SetOperation } from '@yorkie-js-sdk/src/document/operation/set_operation'; @@ -61,23 +64,19 @@ export class RemoveOperation extends Operation { root: CRDTRoot, source: OpSource, ): ExecutionResult | undefined { - const parentObject = root.findByCreatedAt( + const container = root.findByCreatedAt( this.getParentCreatedAt(), ) as CRDTContainer; - if (!parentObject) { + if (!container) { logger.fatal(`fail to find ${this.getParentCreatedAt()}`); } - if (!(parentObject instanceof CRDTContainer)) { - logger.fatal(`only object and array can execute remove: ${parentObject}`); + if (!(container instanceof CRDTContainer)) { + logger.fatal(`only object and array can execute remove: ${container}`); } // NOTE(chacha912): Handle cases where operation cannot be executed during undo and redo. if (source === OpSource.UndoRedo) { - const targetElem = parentObject.getByID(this.createdAt); - if (targetElem?.isRemoved()) { - return; - } - let parent: CRDTContainer | undefined = parentObject; + let parent: CRDTElement | undefined = container.getByID(this.createdAt); while (parent) { if (parent.getRemovedAt()) { return; @@ -85,14 +84,14 @@ export class RemoveOperation extends Operation { parent = root.findElementPairByCreatedAt(parent.getCreatedAt())?.parent; } } - const key = parentObject.subPathOf(this.createdAt); - const reverseOp = this.toReverseOperation(parentObject); + const key = container.subPathOf(this.createdAt); + const reverseOp = this.toReverseOperation(container); - const elem = parentObject.delete(this.createdAt, this.getExecutedAt()); + const elem = container.delete(this.createdAt, this.getExecutedAt()); root.registerRemovedElement(elem); const opInfos: Array = - parentObject instanceof CRDTArray + container instanceof CRDTArray ? [ { type: 'remove', diff --git a/src/document/operation/set_operation.ts b/src/document/operation/set_operation.ts index 0d8d7240d..8d01db463 100644 --- a/src/document/operation/set_operation.ts +++ b/src/document/operation/set_operation.ts @@ -16,10 +16,7 @@ import { logger } from '@yorkie-js-sdk/src/util/logger'; import { TimeTicket } from '@yorkie-js-sdk/src/document/time/ticket'; -import { - CRDTContainer, - CRDTElement, -} from '@yorkie-js-sdk/src/document/crdt/element'; +import { CRDTElement } from '@yorkie-js-sdk/src/document/crdt/element'; import { CRDTRoot } from '@yorkie-js-sdk/src/document/crdt/root'; import { CRDTObject } from '@yorkie-js-sdk/src/document/crdt/object'; import { @@ -67,17 +64,17 @@ export class SetOperation extends Operation { root: CRDTRoot, source: OpSource, ): ExecutionResult | undefined { - const parentObject = root.findByCreatedAt(this.getParentCreatedAt()); - if (!parentObject) { + const obj = root.findByCreatedAt(this.getParentCreatedAt()) as CRDTObject; + if (!obj) { logger.fatal(`fail to find ${this.getParentCreatedAt()}`); } - if (!(parentObject instanceof CRDTObject)) { + if (!(obj instanceof CRDTObject)) { logger.fatal(`fail to execute, only object can execute set`); } - const obj = parentObject as CRDTObject; + // NOTE(chacha912): Handle cases where operation cannot be executed during undo and redo. if (source === OpSource.UndoRedo) { - let parent: CRDTContainer | undefined = obj; + let parent: CRDTElement | undefined = obj; while (parent) { if (parent.getRemovedAt()) { return;