From cc5b0fed079eb4aae7b85ed03cebf791ec36f7ef Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 5 Jun 2025 10:07:00 +0200 Subject: [PATCH] [document-updater] make setDoc aware of tracked deletes in history-ot (#26126) GitOrigin-RevId: efa1a94f2f435058b553f639e43832454c58591d --- services/document-updater/app/js/DiffCodec.js | 54 +++- .../app/js/DocumentManager.js | 5 +- .../acceptance/js/SettingADocumentTests.js | 281 ++++++++++++++++++ 3 files changed, 330 insertions(+), 10 deletions(-) diff --git a/services/document-updater/app/js/DiffCodec.js b/services/document-updater/app/js/DiffCodec.js index 8c574cff70..17da409386 100644 --- a/services/document-updater/app/js/DiffCodec.js +++ b/services/document-updater/app/js/DiffCodec.js @@ -1,3 +1,4 @@ +const OError = require('@overleaf/o-error') const DMP = require('diff-match-patch') const { TextOperation } = require('overleaf-editor-core') const dmp = new DMP() @@ -38,23 +39,62 @@ module.exports = { return ops }, - diffAsHistoryV1EditOperation(before, after) { - const diffs = dmp.diff_main(before, after) + /** + * @param {import("overleaf-editor-core").StringFileData} file + * @param {string} after + * @return {TextOperation} + */ + diffAsHistoryOTEditOperation(file, after) { + const beforeWithoutTrackedDeletes = file.getContent({ + filterTrackedDeletes: true, + }) + const diffs = dmp.diff_main(beforeWithoutTrackedDeletes, after) dmp.diff_cleanupSemantic(diffs) + const trackedChanges = file.trackedChanges.asSorted() + let nextTc = trackedChanges.shift() + const op = new TextOperation() for (const diff of diffs) { - const [type, content] = diff + let [type, content] = diff if (type === this.ADDED) { op.insert(content) - } else if (type === this.REMOVED) { - op.remove(content.length) - } else if (type === this.UNCHANGED) { - op.retain(content.length) + } else if (type === this.REMOVED || type === this.UNCHANGED) { + while (op.baseLength + content.length > nextTc?.range.start) { + if (nextTc.tracking.type === 'delete') { + const untilRange = nextTc.range.start - op.baseLength + if (type === this.REMOVED) { + op.remove(untilRange) + } else if (type === this.UNCHANGED) { + op.retain(untilRange) + } + op.retain(nextTc.range.end - nextTc.range.start) + content = content.slice(untilRange) + } + nextTc = trackedChanges.shift() + } + if (type === this.REMOVED) { + op.remove(content.length) + } else if (type === this.UNCHANGED) { + op.retain(content.length) + } } else { throw new Error('Unknown type') } } + while (nextTc) { + if ( + nextTc.tracking.type !== 'delete' || + nextTc.range.start !== op.baseLength + ) { + throw new OError( + 'StringFileData.trackedChanges out of sync: unexpected range after end of diff', + { nextTc, baseLength: op.baseLength } + ) + } + op.retain(nextTc.range.end - nextTc.range.start) + nextTc = trackedChanges.shift() + } return op }, } diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index 4803056423..6080c1c97d 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -194,9 +194,8 @@ const DocumentManager = { let op if (type === 'history-ot') { const file = StringFileData.fromRaw(oldLines) - const operation = DiffCodec.diffAsHistoryV1EditOperation( - // TODO(24596): tc support for history-ot - file.getContent({ filterTrackedDeletes: true }), + const operation = DiffCodec.diffAsHistoryOTEditOperation( + file, newLines.join('\n') ) if (operation.isNoop()) { diff --git a/services/document-updater/test/acceptance/js/SettingADocumentTests.js b/services/document-updater/test/acceptance/js/SettingADocumentTests.js index fd1851a221..e1bc54dc90 100644 --- a/services/document-updater/test/acceptance/js/SettingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/SettingADocumentTests.js @@ -686,4 +686,285 @@ describe('Setting a document', function () { }) }) }) + + describe('with track changes (history-ot)', function () { + const lines = ['one', 'one and a half', 'two', 'three'] + const userId = DocUpdaterClient.randomId() + const ts = new Date().toISOString() + beforeEach(function (done) { + numberOfReceivedUpdates = 0 + this.newLines = ['one', 'two', 'three'] + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() + this.historyOTUpdate = { + doc: this.doc_id, + op: [ + { + textOperation: [ + 4, + { + r: 'one and a half\n'.length, + tracking: { + type: 'delete', + userId, + ts, + }, + }, + 9, + ], + }, + ], + v: this.version, + meta: { source: 'random-publicId' }, + } + MockWebApi.insertDoc(this.project_id, this.doc_id, { + lines, + version: this.version, + otMigrationStage: 1, + }) + DocUpdaterClient.preloadDoc(this.project_id, this.doc_id, error => { + if (error) { + throw error + } + DocUpdaterClient.sendUpdate( + this.project_id, + this.doc_id, + this.historyOTUpdate, + error => { + if (error) { + throw error + } + DocUpdaterClient.waitForPendingUpdates( + this.project_id, + this.doc_id, + done + ) + } + ) + }) + }) + + afterEach(function () { + MockProjectHistoryApi.flushProject.resetHistory() + MockWebApi.setDocument.resetHistory() + }) + it('should record tracked changes', function (done) { + docUpdaterRedis.get( + Keys.docLines({ doc_id: this.doc_id }), + (error, data) => { + if (error) { + throw error + } + expect(JSON.parse(data)).to.deep.equal({ + content: lines.join('\n'), + trackedChanges: [ + { + range: { + pos: 4, + length: 15, + }, + tracking: { + ts, + type: 'delete', + userId, + }, + }, + ], + }) + done() + } + ) + }) + + it('should apply the change', function (done) { + DocUpdaterClient.getDoc( + this.project_id, + this.doc_id, + (error, res, data) => { + if (error) { + throw error + } + expect(data.lines).to.deep.equal(this.newLines) + done() + } + ) + }) + const cases = [ + { + name: 'when resetting the content', + lines, + want: { + content: 'one\none and a half\none and a half\ntwo\nthree', + trackedChanges: [ + { + range: { + pos: 'one and a half\n'.length + 4, + length: 15, + }, + tracking: { + ts, + type: 'delete', + userId, + }, + }, + ], + }, + }, + { + name: 'when adding content before a tracked delete', + lines: ['one', 'INSERT', 'two', 'three'], + want: { + content: 'one\nINSERT\none and a half\ntwo\nthree', + trackedChanges: [ + { + range: { + pos: 'INSERT\n'.length + 4, + length: 15, + }, + tracking: { + ts, + type: 'delete', + userId, + }, + }, + ], + }, + }, + { + name: 'when adding content after a tracked delete', + lines: ['one', 'two', 'INSERT', 'three'], + want: { + content: 'one\none and a half\ntwo\nINSERT\nthree', + trackedChanges: [ + { + range: { + pos: 4, + length: 15, + }, + tracking: { + ts, + type: 'delete', + userId, + }, + }, + ], + }, + }, + { + name: 'when deleting content before a tracked delete', + lines: ['two', 'three'], + want: { + content: 'one and a half\ntwo\nthree', + trackedChanges: [ + { + range: { + pos: 0, + length: 15, + }, + tracking: { + ts, + type: 'delete', + userId, + }, + }, + ], + }, + }, + { + name: 'when deleting content after a tracked delete', + lines: ['one', 'two'], + want: { + content: 'one\none and a half\ntwo', + trackedChanges: [ + { + range: { + pos: 4, + length: 15, + }, + tracking: { + ts, + type: 'delete', + userId, + }, + }, + ], + }, + }, + { + name: 'when deleting content immediately after a tracked delete', + lines: ['one', 'three'], + want: { + content: 'one\none and a half\nthree', + trackedChanges: [ + { + range: { + pos: 4, + length: 15, + }, + tracking: { + ts, + type: 'delete', + userId, + }, + }, + ], + }, + }, + { + name: 'when deleting content across a tracked delete', + lines: ['onethree'], + want: { + content: 'oneone and a half\nthree', + trackedChanges: [ + { + range: { + pos: 3, + length: 15, + }, + tracking: { + ts, + type: 'delete', + userId, + }, + }, + ], + }, + }, + ] + + for (const { name, lines, want } of cases) { + describe(name, function () { + beforeEach(function (done) { + DocUpdaterClient.setDocLines( + this.project_id, + this.doc_id, + lines, + this.source, + userId, + false, + (error, res, body) => { + if (error) { + return done(error) + } + this.statusCode = res.statusCode + this.body = body + done() + } + ) + }) + it('should update accordingly', function (done) { + docUpdaterRedis.get( + Keys.docLines({ doc_id: this.doc_id }), + (error, data) => { + if (error) { + throw error + } + expect(JSON.parse(data)).to.deep.equal(want) + done() + } + ) + }) + }) + } + }) })