Merge pull request #20799 from overleaf/em-ranges-tracker-sanity-checks

Sanity check for tracked changes in document-updater

GitOrigin-RevId: 5094eee8c279eb194114ac6f7fa36f86c9e16ca7
This commit is contained in:
Eric Mc Sween 2024-10-03 08:17:54 -04:00 committed by Copybot
parent c91d99de80
commit b881a96b84
4 changed files with 84 additions and 3 deletions

View file

@ -264,7 +264,12 @@ const DocumentManager = {
throw new Errors.NotFoundError(`document not found: ${docId}`)
}
const newRanges = RangesManager.acceptChanges(changeIds, ranges)
const newRanges = RangesManager.acceptChanges(
projectId,
docId,
changeIds,
ranges
)
await RedisManager.promises.updateDocument(
projectId,

View file

@ -80,6 +80,8 @@ const RangesManager = {
}
}
sanityCheckTrackedChanges(projectId, docId, rangesTracker.changes)
if (
rangesTracker.changes?.length > RangesManager.MAX_CHANGES ||
rangesTracker.comments?.length > RangesManager.MAX_COMMENTS
@ -127,11 +129,12 @@ const RangesManager = {
return { newRanges, rangesWereCollapsed, historyUpdates }
},
acceptChanges(changeIds, ranges) {
acceptChanges(projectId, docId, 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
},
@ -557,4 +560,66 @@ 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

View file

@ -691,6 +691,8 @@ 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
)
@ -722,7 +724,12 @@ describe('DocumentManager', function () {
it('should apply the accept change to the ranges', function () {
this.RangesManager.acceptChanges
.calledWith(this.change_ids, this.ranges)
.calledWith(
this.project_id,
this.doc_id,
this.change_ids,
this.ranges
)
.should.equal(true)
})
})

View file

@ -668,6 +668,8 @@ 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
)
@ -714,6 +716,8 @@ describe('RangesManager', function () {
this.ranges.changes[4].id,
]
this.result = this.RangesManager.acceptChanges(
this.project_id,
this.doc_id,
this.change_ids,
this.ranges
)