From baa2797e8ffa0e5ce21466a82b3c2ca771148e81 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] 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, }) {