From 8e662b33fba47b9925352696c3dc3acb081ced44 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Mon, 8 Apr 2024 11:07:41 -0400 Subject: [PATCH] Merge pull request #17711 from overleaf/em-resync-tracked-deletes Include tracked deletes in resync doc update GitOrigin-RevId: 410f9435f92ff44184f60a2695be27d7b819a1e2 --- .../app/js/DocumentManager.js | 8 +- .../app/js/ProjectHistoryRedisManager.js | 35 +++++++-- services/document-updater/app/js/Utils.js | 31 +++++++- .../DocumentManager/DocumentManagerTests.js | 10 ++- .../ProjectHistoryRedisManagerTests.js | 77 +++++++++++++++---- .../test/unit/js/UtilsTests.js | 27 +++++++ 6 files changed, 162 insertions(+), 26 deletions(-) create mode 100644 services/document-updater/test/unit/js/UtilsTests.js diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index 0fa2958a20..46c33ef2dc 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -320,7 +320,7 @@ const DocumentManager = { async resyncDocContents(projectId, docId, path) { logger.debug({ projectId, docId, path }, 'start resyncing doc contents') - let { lines, version, projectHistoryId } = + let { lines, ranges, version, projectHistoryId, historyRangesSupport } = await RedisManager.promises.getDoc(projectId, docId) // To avoid issues where the same docId appears with different paths, @@ -333,7 +333,7 @@ const DocumentManager = { { projectId, docId }, 'resyncing doc contents - not found in redis - retrieving from web' ) - ;({ lines, version, projectHistoryId } = + ;({ lines, ranges, version, projectHistoryId, historyRangesSupport } = await PersistenceManager.promises.getDoc(projectId, docId, { peek: true, })) @@ -349,9 +349,11 @@ const DocumentManager = { projectHistoryId, docId, lines, + ranges, version, // use the path from the resyncProjectStructure update - path + path, + historyRangesSupport ) }, diff --git a/services/document-updater/app/js/ProjectHistoryRedisManager.js b/services/document-updater/app/js/ProjectHistoryRedisManager.js index 9babdd26a3..fe548e749d 100644 --- a/services/document-updater/app/js/ProjectHistoryRedisManager.js +++ b/services/document-updater/app/js/ProjectHistoryRedisManager.js @@ -1,3 +1,5 @@ +// @ts-check + const Settings = require('@overleaf/settings') const { callbackifyAll } = require('@overleaf/promise-utils') const projectHistoryKeys = Settings.redis?.project_history?.key_schema @@ -7,8 +9,13 @@ const rclient = require('@overleaf/redis-wrapper').createClient( const logger = require('@overleaf/logger') const metrics = require('./Metrics') const { docIsTooLarge } = require('./Limits') +const { addTrackedDeletesToContent } = require('./Utils') const OError = require('@overleaf/o-error') +/** + * @typedef {import('./types').Ranges} Ranges + */ + const ProjectHistoryRedisManager = { async queueOps(projectId, ...ops) { // Record metric for ops pushed onto queue @@ -119,23 +126,41 @@ const ProjectHistoryRedisManager = { return await ProjectHistoryRedisManager.queueOps(projectId, jsonUpdate) }, + /** + * Add a resync doc update to the project-history queue + * + * @param {string} projectId + * @param {string} projectHistoryId + * @param {string} docId + * @param {string[]} lines + * @param {Ranges} ranges + * @param {number} version + * @param {string} pathname + * @param {boolean} historyRangesSupport + * @return {Promise} the number of ops added + */ async queueResyncDocContent( projectId, projectHistoryId, docId, lines, + ranges, version, - pathname + pathname, + historyRangesSupport ) { logger.debug( { projectId, docId, lines, version, pathname }, 'queue doc content resync' ) + + let content = lines.join('\n') + if (historyRangesSupport) { + content = addTrackedDeletesToContent(content, ranges.changes ?? []) + } + const projectUpdate = { - resyncDocContent: { - content: lines.join('\n'), - version, - }, + resyncDocContent: { content, version }, projectHistoryId, path: pathname, doc: docId, diff --git a/services/document-updater/app/js/Utils.js b/services/document-updater/app/js/Utils.js index 9965f22b0f..115407181d 100644 --- a/services/document-updater/app/js/Utils.js +++ b/services/document-updater/app/js/Utils.js @@ -5,6 +5,7 @@ * @typedef {import('./types').DeleteOp} DeleteOp * @typedef {import('./types').InsertOp} InsertOp * @typedef {import('./types').Op} Op + * @typedef {import('./types').TrackedChange} TrackedChange */ /** @@ -37,4 +38,32 @@ function isComment(op) { return 'c' in op && op.c != null } -module.exports = { isInsert, isDelete, isComment } +/** + * Adds given tracked deletes to the given content. + * + * The history system includes tracked deletes in the document content. + * + * @param {string} content + * @param {TrackedChange[]} trackedChanges + * @return {string} content for the history service + */ +function addTrackedDeletesToContent(content, trackedChanges) { + let cursor = 0 + let result = '' + for (const change of trackedChanges) { + if (isDelete(change.op)) { + // Add the content before the tracked delete + result += content.slice(cursor, change.op.p) + cursor = change.op.p + // Add the content of the tracked delete + result += change.op.d + } + } + + // Add the content after all tracked deletes + result += content.slice(cursor) + + return result +} + +module.exports = { isInsert, isDelete, isComment, addTrackedDeletesToContent } diff --git a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js index 7648ecc4bb..2d3f8cd4ba 100644 --- a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js +++ b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js @@ -987,6 +987,7 @@ describe('DocumentManager', function () { ranges: this.ranges, pathname: this.pathname, projectHistoryId: this.projectHistoryId, + historyRangesSupport: this.historyRangesSupport, }) await this.DocumentManager.promises.resyncDocContents( this.project_id, @@ -1008,8 +1009,10 @@ describe('DocumentManager', function () { this.projectHistoryId, this.doc_id, this.lines, + this.ranges, this.version, - this.pathnameFromProjectStructureUpdate + this.pathnameFromProjectStructureUpdate, + this.historyRangesSupport ) .should.equal(true) }) @@ -1025,6 +1028,7 @@ describe('DocumentManager', function () { ranges: this.ranges, pathname: this.pathname, projectHistoryId: this.projectHistoryId, + historyRangesSupport: this.historyRangesSupport, }) await this.DocumentManager.promises.resyncDocContents( this.project_id, @@ -1052,8 +1056,10 @@ describe('DocumentManager', function () { this.projectHistoryId, this.doc_id, this.lines, + this.ranges, this.version, - this.pathnameFromProjectStructureUpdate + this.pathnameFromProjectStructureUpdate, + this.historyRangesSupport ) .should.equal(true) }) diff --git a/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js b/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js index dcb2dbe5a9..fd1d4b3a35 100644 --- a/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js +++ b/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js @@ -177,22 +177,12 @@ describe('ProjectHistoryRedisManager', function () { beforeEach(function () { this.doc_id = 1234 this.lines = ['one', 'two'] + this.ranges = { + changes: [{ op: { i: 'ne', p: 1 } }, { op: { d: 'deleted', p: 3 } }], + } this.version = 2 this.pathname = '/path' - this.update = { - resyncDocContent: { - content: this.lines.join('\n'), - version: this.version, - }, - projectHistoryId: this.projectHistoryId, - path: this.pathname, - doc: this.doc_id, - meta: { - ts: new Date(), - }, - } - this.ProjectHistoryRedisManager.promises.queueOps = sinon .stub() .resolves() @@ -200,15 +190,29 @@ describe('ProjectHistoryRedisManager', function () { describe('with a good doc', function () { beforeEach(async function () { + this.update = { + resyncDocContent: { + content: 'one\ntwo', + version: this.version, + }, + projectHistoryId: this.projectHistoryId, + path: this.pathname, + doc: this.doc_id, + meta: { ts: new Date() }, + } + await this.ProjectHistoryRedisManager.promises.queueResyncDocContent( this.project_id, this.projectHistoryId, this.doc_id, this.lines, + this.ranges, this.version, - this.pathname + this.pathname, + false ) }) + it('should check if the doc is too large', function () { this.Limits.docIsTooLarge .calledWith( @@ -235,8 +239,10 @@ describe('ProjectHistoryRedisManager', function () { this.projectHistoryId, this.doc_id, this.lines, + this.ranges, this.version, - this.pathname + this.pathname, + false ) ).to.be.rejected }) @@ -247,6 +253,47 @@ describe('ProjectHistoryRedisManager', function () { ) }) }) + + describe('when history ranges support is enabled', function () { + beforeEach(async function () { + this.update = { + resyncDocContent: { + content: 'onedeleted\ntwo', + version: this.version, + }, + projectHistoryId: this.projectHistoryId, + path: this.pathname, + doc: this.doc_id, + meta: { ts: new Date() }, + } + + await this.ProjectHistoryRedisManager.promises.queueResyncDocContent( + this.project_id, + this.projectHistoryId, + this.doc_id, + this.lines, + this.ranges, + this.version, + this.pathname, + true + ) + }) + + it('should include tracked deletes in the update', function () { + this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWithExactly( + this.project_id, + JSON.stringify(this.update) + ) + }) + + it('should check the doc length without tracked deletes', function () { + this.Limits.docIsTooLarge.should.have.been.calledWith( + JSON.stringify(this.update).length, + this.lines, + this.settings.max_doc_length + ) + }) + }) }) }) }) diff --git a/services/document-updater/test/unit/js/UtilsTests.js b/services/document-updater/test/unit/js/UtilsTests.js new file mode 100644 index 0000000000..553b90159a --- /dev/null +++ b/services/document-updater/test/unit/js/UtilsTests.js @@ -0,0 +1,27 @@ +// @ts-check + +const { expect } = require('chai') +const Utils = require('../../../app/js/Utils') + +describe('Utils', function () { + describe('addTrackedDeletesToContent', function () { + it("doesn't modify text without tracked deletes", function () { + const content = 'the quick brown fox' + const trackedChanges = [] + const result = Utils.addTrackedDeletesToContent(content, trackedChanges) + expect(result).to.equal(content) + }) + + it('adds tracked deletes to text but skips tracked inserts', function () { + const content = 'the brown fox jumps over the dog' + const metadata = { user_id: 'user1', ts: new Date().toString() } + const trackedChanges = [ + { id: 'tc1', op: { d: 'quick ', p: 4 }, metadata }, + { id: 'tc2', op: { i: 'brown ', p: 5 }, metadata }, + { id: 'tc3', op: { d: 'lazy ', p: 29 }, metadata }, + ] + const result = Utils.addTrackedDeletesToContent(content, trackedChanges) + expect(result).to.equal('the quick brown fox jumps over the lazy dog') + }) + }) +})