From 24d7e3b98712e115212efdc367bc8ed0666e62b5 Mon Sep 17 00:00:00 2001 From: Yourim Cha Date: Wed, 22 Nov 2023 18:58:47 +0900 Subject: [PATCH] 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); + }); }); });