From a93223c70bcd597401f51b1925588bb7d8da1d63 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Thu, 3 Oct 2024 09:47:04 -0400 Subject: [PATCH] Merge pull request #20826 from overleaf/revert-20799-em-ranges-tracker-sanity-checks Revert "Sanity check for tracked changes in document-updater" GitOrigin-RevId: 7876d57298d0f5dbd54929fdf69bce2976f16a9f --- .../app/js/DocumentManager.js | 7 +- .../document-updater/app/js/RangesManager.js | 67 +------------------ .../DocumentManager/DocumentManagerTests.js | 9 +-- .../js/RangesManager/RangesManagerTests.js | 4 -- 4 files changed, 3 insertions(+), 84 deletions(-) diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index 149a63bcc9..157a99ccf6 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -264,12 +264,7 @@ const DocumentManager = { throw new Errors.NotFoundError(`document not found: ${docId}`) } - const newRanges = RangesManager.acceptChanges( - projectId, - docId, - changeIds, - ranges - ) + const newRanges = RangesManager.acceptChanges(changeIds, ranges) await RedisManager.promises.updateDocument( projectId, diff --git a/services/document-updater/app/js/RangesManager.js b/services/document-updater/app/js/RangesManager.js index d71411fe8d..7dfa462ce4 100644 --- a/services/document-updater/app/js/RangesManager.js +++ b/services/document-updater/app/js/RangesManager.js @@ -80,8 +80,6 @@ const RangesManager = { } } - sanityCheckTrackedChanges(projectId, docId, rangesTracker.changes) - if ( rangesTracker.changes?.length > RangesManager.MAX_CHANGES || rangesTracker.comments?.length > RangesManager.MAX_COMMENTS @@ -129,12 +127,11 @@ const RangesManager = { return { newRanges, rangesWereCollapsed, historyUpdates } }, - acceptChanges(projectId, docId, changeIds, ranges) { + acceptChanges(changeIds, ranges) { const { changes, comments } = ranges logger.debug(`accepting ${changeIds.length} changes in ranges`) const rangesTracker = new RangesTracker(changes, comments) rangesTracker.removeChangeIds(changeIds) - sanityCheckTrackedChanges(projectId, docId, rangesTracker.changes) const newRanges = RangesManager._getRanges(rangesTracker) return newRanges }, @@ -560,66 +557,4 @@ function getCroppedCommentOps(op, comments) { return historyCommentOps } -/** - * Check some tracked changes assumptions: - * - * - Tracked changes can't be empty - * - Tracked inserts can't overlap with another tracked change - * - There can't be two tracked deletes at the same position - * - Ranges should be ordered by position, deletes before inserts - * - Every tracked change id should be unique - * - * If any assumption isn't upheld, log a warning. - * - * @param {string} projectId - * @param {string} docId - * @param {TrackedChange[]} changes - */ -function sanityCheckTrackedChanges(projectId, docId, changes) { - const idsSeen = new Set() - let lastDeletePos = -1 // allow a tracked delete at position 0 - let lastInsertEnd = 0 - let ok = true - for (const change of changes) { - if (idsSeen.has(change.id)) { - ok = false - break - } - idsSeen.add(change.id) - - const op = change.op - if ('i' in op) { - if (op.i.length === 0 || op.p < lastDeletePos || op.p < lastInsertEnd) { - ok = false - break - } - lastInsertEnd = op.p + op.i.length - } else if ('d' in op) { - if (op.d.length === 0 || op.p <= lastDeletePos || op.p < lastInsertEnd) { - ok = false - break - } - lastDeletePos = op.p - } - } - - if (ok) { - return - } - - const changeRanges = [] - for (const change of changes) { - if ('i' in change.op) { - changeRanges.push({ p: change.op.p, i: change.op.i.length }) - } else if ('d' in change.op) { - changeRanges.push({ p: change.op.p, d: change.op.d.length }) - } - } - - logger.warn( - { projectId, docId, changes: changeRanges }, - 'Malformed tracked changes detected' - ) -} - module.exports = RangesManager diff --git a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js index 75b7209b75..a0c379a5be 100644 --- a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js +++ b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js @@ -691,8 +691,6 @@ describe('DocumentManager', function () { it('should apply the accept change to the ranges', function () { this.RangesManager.acceptChanges.should.have.been.calledWith( - this.project_id, - this.doc_id, [this.change_id], this.ranges ) @@ -724,12 +722,7 @@ describe('DocumentManager', function () { it('should apply the accept change to the ranges', function () { this.RangesManager.acceptChanges - .calledWith( - this.project_id, - this.doc_id, - this.change_ids, - this.ranges - ) + .calledWith(this.change_ids, this.ranges) .should.equal(true) }) }) diff --git a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js index a7125219bb..67ac6cc175 100644 --- a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js +++ b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js @@ -668,8 +668,6 @@ describe('RangesManager', function () { beforeEach(function () { this.change_ids = [this.ranges.changes[1].id] this.result = this.RangesManager.acceptChanges( - this.project_id, - this.doc_id, this.change_ids, this.ranges ) @@ -716,8 +714,6 @@ describe('RangesManager', function () { this.ranges.changes[4].id, ] this.result = this.RangesManager.acceptChanges( - this.project_id, - this.doc_id, this.change_ids, this.ranges )