From 21f6b2f1e018dc3882b10af8538bc1422545a158 Mon Sep 17 00:00:00 2001 From: Yourim Cha Date: Fri, 17 Nov 2023 17:49:36 +0900 Subject: [PATCH] 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 ({