From 6d216d4738ec4da67e8b6995c39e9f2f9bb3006a Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Wed, 22 May 2024 08:15:07 -0400 Subject: [PATCH] Merge pull request #18355 from overleaf/em-resync-tracked-changes Handle tracked changes during resyncs GitOrigin-RevId: 1d5b16a4cb17226da184a5430ebbcfc79ad9c7ce --- .../project-history/app/js/SyncManager.js | 257 ++++++++++++++++- .../app/js/UpdateTranslator.js | 40 +-- services/project-history/app/js/Utils.js | 41 +++ services/project-history/app/js/types.ts | 30 +- .../unit/js/SyncManager/SyncManagerTests.js | 260 ++++++++++++++++-- 5 files changed, 545 insertions(+), 83 deletions(-) create mode 100644 services/project-history/app/js/Utils.js diff --git a/services/project-history/app/js/SyncManager.js b/services/project-history/app/js/SyncManager.js index 7864029108..144f62bc24 100644 --- a/services/project-history/app/js/SyncManager.js +++ b/services/project-history/app/js/SyncManager.js @@ -19,12 +19,19 @@ import * as ErrorRecorder from './ErrorRecorder.js' import * as RedisManager from './RedisManager.js' import * as HistoryStoreManager from './HistoryStoreManager.js' import * as HashManager from './HashManager.js' +import { isInsert } from './Utils.js' /** * @typedef {import('overleaf-editor-core').Comment} HistoryComment + * @typedef {import('overleaf-editor-core').TrackedChange} HistoryTrackedChange * @typedef {import('./types').Comment} Comment * @typedef {import('./types').Entity} Entity * @typedef {import('./types').ResyncDocContentUpdate} ResyncDocContentUpdate + * @typedef {import('./types').RetainOp} RetainOp + * @typedef {import('./types').TrackedChange} TrackedChange + * @typedef {import('./types').TrackedChangeTransition} TrackedChangeTransition + * @typedef {import('./types').TrackingDirective} TrackingDirective + * @typedef {import('./types').TrackingType} TrackingType * @typedef {import('./types').Update} Update */ const MAX_RESYNC_HISTORY_RECORDS = 100 // keep this many records of previous resyncs @@ -639,12 +646,18 @@ class SyncUpdateExpander { } const persistedComments = file.getComments().toArray() - await this.queueUpdateForOutOfSyncComments( + await this.queueUpdatesForOutOfSyncComments( update, pathname, - persistedContent, persistedComments ) + + const persistedChanges = file.getTrackedChanges().asSorted() + await this.queueUpdatesForOutOfSyncTrackedChanges( + update, + pathname, + persistedChanges + ) } /** @@ -694,19 +707,14 @@ class SyncUpdateExpander { } /** - * Queue update for out of sync comments + * Queue updates for out of sync comments * * @param {ResyncDocContentUpdate} update * @param {string} pathname - * @param {string} persistedContent * @param {HistoryComment[]} persistedComments */ - async queueUpdateForOutOfSyncComments( - update, - pathname, - persistedContent, - persistedComments - ) { + async queueUpdatesForOutOfSyncComments(update, pathname, persistedComments) { + const expectedContent = update.resyncDocContent.content const expectedComments = update.resyncDocContent.ranges?.comments ?? [] const resolvedComments = new Set( update.resyncDocContent.resolvedComments ?? [] @@ -760,12 +768,129 @@ class SyncUpdateExpander { origin: this.origin, ts: update.meta.ts, pathname, - doc_length: persistedContent.length, + doc_length: expectedContent.length, }, }) } } } + + /** + * Queue updates for out of sync tracked changes + * + * @param {ResyncDocContentUpdate} update + * @param {string} pathname + * @param {readonly HistoryTrackedChange[]} persistedChanges + */ + async queueUpdatesForOutOfSyncTrackedChanges( + update, + pathname, + persistedChanges + ) { + const expectedChanges = update.resyncDocContent.ranges?.changes ?? [] + const expectedContent = update.resyncDocContent.content + + /** + * A cursor on the expected content + */ + let cursor = 0 + + /** + * The persisted tracking at cursor + * + * @type {TrackingDirective} + */ + let persistedTracking = { type: 'none' } + + /** + * The expected tracking at cursor + * + * @type {TrackingDirective} + */ + let expectedTracking = { type: 'none' } + + /** + * The retain ops for the update + * + * @type {RetainOp[]} + */ + const ops = [] + + /** + * The retain op being built + * + * @type {RetainOp | null} + */ + let currentOp = null + + for (const transition of getTrackedChangesTransitions( + persistedChanges, + expectedChanges, + expectedContent.length + )) { + if (transition.pos > cursor) { + // The next transition will move the cursor. Decide what to do with the interval. + if (trackingDirectivesEqual(expectedTracking, persistedTracking)) { + // Expected tracking and persisted tracking are in sync. Emit the + // current op and skip this interval. + if (currentOp != null) { + ops.push(currentOp) + currentOp = null + } + } else { + // Expected tracking and persisted tracking are different. + const retainedText = expectedContent.slice(cursor, transition.pos) + if ( + currentOp?.tracking != null && + trackingDirectivesEqual(expectedTracking, currentOp.tracking) + ) { + // The current op has the right tracking. Extend it. + currentOp.r += retainedText + } else { + // The current op doesn't have the right tracking. Emit the current + // op and start a new one. + if (currentOp != null) { + ops.push(currentOp) + } + currentOp = { + r: retainedText, + p: cursor, + tracking: expectedTracking, + } + } + } + + // Advance cursor + cursor = transition.pos + } + + // Update the expected and persisted tracking + if (transition.stage === 'persisted') { + persistedTracking = transition.tracking + } else { + expectedTracking = transition.tracking + } + } + + // Emit the last op + if (currentOp != null) { + ops.push(currentOp) + } + + if (ops.length > 0) { + this.expandedUpdates.push({ + doc: update.doc, + op: ops, + meta: { + resync: true, + origin: this.origin, + ts: update.meta.ts, + pathname, + doc_length: expectedContent.length, + }, + }) + } + } } /** @@ -788,6 +913,116 @@ function commentRangesAreInSync(persistedComment, expectedComment) { ) } +/** + * Iterates through expected tracked changes and persisted tracked changes and + * returns all transitions, sorted by position. + * + * @param {readonly HistoryTrackedChange[]} persistedChanges + * @param {TrackedChange[]} expectedChanges + * @param {number} docLength + */ +function getTrackedChangesTransitions( + persistedChanges, + expectedChanges, + docLength +) { + /** @type {TrackedChangeTransition[]} */ + const transitions = [] + + for (const change of persistedChanges) { + transitions.push({ + stage: 'persisted', + pos: change.range.start, + tracking: { + type: change.tracking.type, + userId: change.tracking.userId, + ts: change.tracking.ts.toISOString(), + }, + }) + transitions.push({ + stage: 'persisted', + pos: change.range.end, + tracking: { type: 'none' }, + }) + } + + for (const change of expectedChanges) { + const op = change.op + const pos = op.hpos ?? op.p + if (isInsert(op)) { + transitions.push({ + stage: 'expected', + pos, + tracking: { + type: 'insert', + userId: change.metadata.user_id, + ts: change.metadata.ts, + }, + }) + transitions.push({ + stage: 'expected', + pos: pos + op.i.length, + tracking: { type: 'none' }, + }) + } else { + transitions.push({ + stage: 'expected', + pos, + tracking: { + type: 'delete', + userId: change.metadata.user_id, + ts: change.metadata.ts, + }, + }) + transitions.push({ + stage: 'expected', + pos: pos + op.d.length, + tracking: { type: 'none' }, + }) + } + } + + transitions.push({ + stage: 'expected', + pos: docLength, + tracking: { type: 'none' }, + }) + + transitions.sort((a, b) => { + if (a.pos < b.pos) { + return -1 + } else if (a.pos > b.pos) { + return 1 + } else if (a.tracking.type === 'none' && b.tracking.type !== 'none') { + // none type comes before other types so that it can be overridden at the + // same position + return -1 + } else if (a.tracking.type !== 'none' && b.tracking.type === 'none') { + // none type comes before other types so that it can be overridden at the + // same position + return 1 + } else { + return 0 + } + }) + + return transitions +} + +/** + * Returns true if both tracking directives are equal + * + * @param {TrackingDirective} a + * @param {TrackingDirective} b + */ +function trackingDirectivesEqual(a, b) { + if (a.type === 'none') { + return b.type === 'none' + } else { + return a.type === b.type && a.userId === b.userId && a.ts === b.ts + } +} + // EXPORTS const startResyncCb = callbackify(startResync) diff --git a/services/project-history/app/js/UpdateTranslator.js b/services/project-history/app/js/UpdateTranslator.js index c272394f95..ad5a5ce4ce 100644 --- a/services/project-history/app/js/UpdateTranslator.js +++ b/services/project-history/app/js/UpdateTranslator.js @@ -4,19 +4,17 @@ import _ from 'lodash' import Core from 'overleaf-editor-core' import * as Errors from './Errors.js' import * as OperationsCompressor from './OperationsCompressor.js' +import { isInsert, isRetain, isDelete, isComment } from './Utils.js' /** * @typedef {import('./types').AddDocUpdate} AddDocUpdate * @typedef {import('./types').AddFileUpdate} AddFileUpdate - * @typedef {import('./types').CommentOp} CommentOp * @typedef {import('./types').DeleteCommentUpdate} DeleteCommentUpdate - * @typedef {import('./types').DeleteOp} DeleteOp - * @typedef {import('./types').InsertOp} InsertOp - * @typedef {import('./types').RetainOp} RetainOp * @typedef {import('./types').Op} Op * @typedef {import('./types').RawScanOp} RawScanOp * @typedef {import('./types').RenameUpdate} RenameUpdate * @typedef {import('./types').TextUpdate} TextUpdate + * @typedef {import('./types').TrackingDirective} TrackingDirective * @typedef {import('./types').TrackingProps} TrackingProps * @typedef {import('./types').SetCommentStateUpdate} SetCommentStateUpdate * @typedef {import('./types').Update} Update @@ -405,7 +403,7 @@ class OperationsBuilder { /** * @param {number} length * @param {object} opts - * @param {TrackingProps} [opts.tracking] + * @param {TrackingDirective} [opts.tracking] */ retain(length, opts = {}) { if (opts.tracking) { @@ -461,35 +459,3 @@ class OperationsBuilder { return this.operations } } - -/** - * @param {Op} op - * @returns {op is InsertOp} - */ -function isInsert(op) { - return 'i' in op && op.i != null -} - -/** - * @param {Op} op - * @returns {op is RetainOp} - */ -function isRetain(op) { - return 'r' in op && op.r != null -} - -/** - * @param {Op} op - * @returns {op is DeleteOp} - */ -function isDelete(op) { - return 'd' in op && op.d != null -} - -/** - * @param {Op} op - * @returns {op is CommentOp} - */ -function isComment(op) { - return 'c' in op && op.c != null && 't' in op && op.t != null -} diff --git a/services/project-history/app/js/Utils.js b/services/project-history/app/js/Utils.js new file mode 100644 index 0000000000..59eaf43683 --- /dev/null +++ b/services/project-history/app/js/Utils.js @@ -0,0 +1,41 @@ +// @ts-check + +/** + * @typedef {import('./types').CommentOp} CommentOp + * @typedef {import('./types').DeleteOp} DeleteOp + * @typedef {import('./types').InsertOp} InsertOp + * @typedef {import('./types').Op} Op + * @typedef {import('./types').RetainOp} RetainOp + */ + +/** + * @param {Op} op + * @returns {op is InsertOp} + */ +export function isInsert(op) { + return 'i' in op && op.i != null +} + +/** + * @param {Op} op + * @returns {op is RetainOp} + */ +export function isRetain(op) { + return 'r' in op && op.r != null +} + +/** + * @param {Op} op + * @returns {op is DeleteOp} + */ +export function isDelete(op) { + return 'd' in op && op.d != null +} + +/** + * @param {Op} op + * @returns {op is CommentOp} + */ +export function isComment(op) { + return 'c' in op && op.c != null && 't' in op && op.t != null +} diff --git a/services/project-history/app/js/types.ts b/services/project-history/app/js/types.ts index 5614657836..4e6c6a88a4 100644 --- a/services/project-history/app/js/types.ts +++ b/services/project-history/app/js/types.ts @@ -97,7 +97,7 @@ export type RetainOp = { r: string p: number hpos?: number - tracking?: TrackingProps + tracking?: TrackingDirective } export type InsertOp = { @@ -140,20 +140,22 @@ export type RawOrigin = { kind: string } -export type TrackingProps = - | { type: 'none' } - | { - type: 'insert' | 'delete' - userId: string - ts: string - } +export type TrackingProps = { + type: 'insert' | 'delete' + userId: string + ts: string +} + +export type TrackingDirective = TrackingProps | { type: 'none' } + +export type TrackingType = 'insert' | 'delete' | 'none' export type RawScanOp = | number | string - | { r: number; tracking?: TrackingProps } + | { r: number; tracking?: TrackingDirective } | { i: string; tracking?: TrackingProps; commentIds?: string[] } - | { d: number; tracking?: TrackingProps } + | { d: number } export type TrackedChangeSnapshot = { op: { @@ -207,9 +209,15 @@ export type Comment = { export type TrackedChange = { id: string - op: Op + op: InsertOp | DeleteOp metadata: { user_id: string ts: string } } + +export type TrackedChangeTransition = { + pos: number + tracking: TrackingDirective + stage: 'persisted' | 'expected' +} diff --git a/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js b/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js index fcbc0423cc..0347df979e 100644 --- a/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js +++ b/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js @@ -2,20 +2,20 @@ import sinon from 'sinon' import { expect } from 'chai' import mongodb from 'mongodb-legacy' import tk from 'timekeeper' -import { Comment, Range } from 'overleaf-editor-core' +import { Comment, TrackedChange, Range } from 'overleaf-editor-core' import { strict as esmock } from 'esmock' const { ObjectId } = mongodb const MODULE_PATH = '../../../../app/js/SyncManager.js' - -const timestamp = new Date() +const TIMESTAMP = new Date().toISOString() +const USER_ID = 'user-id' function resyncProjectStructureUpdate(docs, files) { return { resyncProjectStructure: { docs, files }, meta: { - ts: timestamp, + ts: TIMESTAMP, }, } } @@ -37,7 +37,7 @@ function docContentSyncUpdate( }, meta: { - ts: timestamp, + ts: TIMESTAMP, }, } } @@ -46,12 +46,21 @@ function makeComment(commentId, pos, text) { return { id: commentId, op: { p: pos, c: text, t: commentId }, - meta: { - ts: timestamp, + metadata: { + user_id: USER_ID, + ts: TIMESTAMP, }, } } +function makeTrackedChange(id, op) { + return { + id, + op, + metadata: { user_id: USER_ID, ts: TIMESTAMP }, + } +} + describe('SyncManager', function () { beforeEach(async function () { this.now = new Date() @@ -476,7 +485,7 @@ describe('SyncManager', function () { doc: 'doc-id', path: 'main.tex', } - this.persistedDocContent = 'the quick brown fox jumps over the lazy fox' + this.persistedDocContent = 'the quick brown fox jumps over the lazy dog' this.persistedFile = { file: 'file-id', path: '1.png', @@ -488,6 +497,9 @@ describe('SyncManager', function () { getComments: sinon .stub() .returns({ toArray: sinon.stub().returns([]) }), + getTrackedChanges: sinon + .stub() + .returns({ asSorted: sinon.stub().returns([]) }), getHash: sinon.stub().returns(null), } this.fileMap = { @@ -566,7 +578,7 @@ describe('SyncManager', function () { new_pathname: '', meta: { resync: true, - ts: timestamp, + ts: TIMESTAMP, origin: { kind: 'history-resync' }, }, }, @@ -590,7 +602,7 @@ describe('SyncManager', function () { new_pathname: '', meta: { resync: true, - ts: timestamp, + ts: TIMESTAMP, origin: { kind: 'history-resync' }, }, }, @@ -625,7 +637,7 @@ describe('SyncManager', function () { url: newFile.url, meta: { resync: true, - ts: timestamp, + ts: TIMESTAMP, origin: { kind: 'history-resync' }, }, }, @@ -659,7 +671,7 @@ describe('SyncManager', function () { docLines: '', meta: { resync: true, - ts: timestamp, + ts: TIMESTAMP, origin: { kind: 'history-resync' }, }, }, @@ -694,7 +706,7 @@ describe('SyncManager', function () { new_pathname: '', meta: { resync: true, - ts: timestamp, + ts: TIMESTAMP, origin: { kind: 'history-resync' }, }, }, @@ -704,7 +716,7 @@ describe('SyncManager', function () { url: fileWichWasADoc.url, meta: { resync: true, - ts: timestamp, + ts: TIMESTAMP, origin: { kind: 'history-resync' }, }, }, @@ -791,7 +803,7 @@ describe('SyncManager', function () { new_pathname: '', meta: { resync: true, - ts: timestamp, + ts: TIMESTAMP, origin: { kind: 'history-resync' }, }, }, @@ -801,7 +813,7 @@ describe('SyncManager', function () { url: persistedFileWithNewContent.url, meta: { resync: true, - ts: timestamp, + ts: TIMESTAMP, origin: { kind: 'history-resync' }, }, }, @@ -916,7 +928,7 @@ describe('SyncManager', function () { pathname: this.persistedDoc.path, doc_length: this.persistedDocContent.length, resync: true, - ts: timestamp, + ts: TIMESTAMP, origin: { kind: 'history-resync' }, }, }, @@ -953,7 +965,7 @@ describe('SyncManager', function () { docLines: '', meta: { resync: true, - ts: timestamp, + ts: TIMESTAMP, origin: { kind: 'history-resync' }, }, }, @@ -964,7 +976,7 @@ describe('SyncManager', function () { pathname: newDoc.path, doc_length: 0, resync: true, - ts: timestamp, + ts: TIMESTAMP, origin: { kind: 'history-resync' }, }, }, @@ -1023,7 +1035,7 @@ describe('SyncManager', function () { pathname: this.persistedDoc.path, doc_length: this.persistedDocContent.length, resync: true, - ts: timestamp, + ts: TIMESTAMP, origin: { kind: 'history-resync' }, }, }, @@ -1060,7 +1072,7 @@ describe('SyncManager', function () { pathname: this.persistedDoc.path, doc_length: 'stored content'.length, resync: true, - ts: timestamp, + ts: TIMESTAMP, origin: { kind: 'history-resync' }, }, }, @@ -1143,7 +1155,7 @@ describe('SyncManager', function () { }, pathname: this.persistedDoc.path, resync: true, - ts: timestamp, + ts: TIMESTAMP, doc_length: this.persistedDocContent.length, }, }, @@ -1178,7 +1190,7 @@ describe('SyncManager', function () { kind: 'history-resync', }, resync: true, - ts: timestamp, + ts: TIMESTAMP, }, }, ]) @@ -1218,7 +1230,7 @@ describe('SyncManager', function () { kind: 'history-resync', }, resync: true, - ts: timestamp, + ts: TIMESTAMP, pathname: this.persistedDoc.path, doc_length: this.persistedDocContent.length, }, @@ -1259,5 +1271,205 @@ describe('SyncManager', function () { ]) }) }) + + describe('syncing tracked changes', function () { + beforeEach(function () { + this.loadedSnapshotDoc.getTrackedChanges.returns({ + asSorted: sinon.stub().returns([ + new TrackedChange(new Range(4, 6), { + type: 'delete', + userId: USER_ID, + ts: new Date(TIMESTAMP), + }), + new TrackedChange(new Range(10, 6), { + type: 'insert', + userId: USER_ID, + ts: new Date(TIMESTAMP), + }), + new TrackedChange(new Range(20, 6), { + type: 'delete', + userId: USER_ID, + ts: new Date(TIMESTAMP), + }), + new TrackedChange(new Range(40, 3), { + type: 'insert', + userId: USER_ID, + ts: new Date(TIMESTAMP), + }), + ]), + }) + this.changes = [ + makeTrackedChange('td1', { p: 4, d: 'quick ' }), + makeTrackedChange('ti1', { p: 4, hpos: 10, i: 'brown ' }), + makeTrackedChange('td2', { p: 14, hpos: 20, d: 'jumps ' }), + makeTrackedChange('ti2', { p: 28, hpos: 40, i: 'dog' }), + ] + }) + + it('does nothing if tracked changes have not changed', async function () { + const updates = [ + docContentSyncUpdate(this.persistedDoc, this.persistedDocContent, { + changes: this.changes, + }), + ] + const expandedUpdates = + await this.SyncManager.promises.expandSyncUpdates( + this.projectId, + this.historyId, + updates, + this.extendLock + ) + expect(expandedUpdates).to.deep.equal([]) + }) + + it('adds new tracked changes', async function () { + this.changes.splice( + 3, + 0, + makeTrackedChange('td3', { p: 29, hpos: 35, d: 'lazy ' }) + ) + const updates = [ + docContentSyncUpdate(this.persistedDoc, this.persistedDocContent, { + changes: this.changes, + }), + ] + const expandedUpdates = + await this.SyncManager.promises.expandSyncUpdates( + this.projectId, + this.historyId, + updates, + this.extendLock + ) + expect(expandedUpdates).to.deep.equal([ + { + doc: this.persistedDoc.doc, + op: [ + { + p: 35, + r: 'lazy ', + tracking: { + type: 'delete', + userId: USER_ID, + ts: TIMESTAMP, + }, + }, + ], + meta: { + origin: { + kind: 'history-resync', + }, + pathname: this.persistedDoc.path, + resync: true, + ts: TIMESTAMP, + doc_length: this.persistedDocContent.length, + }, + }, + ]) + }) + + it('removes extra tracked changes', async function () { + this.changes.splice(0, 1) + const updates = [ + docContentSyncUpdate(this.persistedDoc, this.persistedDocContent, { + changes: this.changes, + }), + ] + const expandedUpdates = + await this.SyncManager.promises.expandSyncUpdates( + this.projectId, + this.historyId, + updates, + this.extendLock + ) + expect(expandedUpdates).to.deep.equal([ + { + doc: this.persistedDoc.doc, + op: [ + { + p: 4, + r: 'quick ', + tracking: { type: 'none' }, + }, + ], + meta: { + origin: { + kind: 'history-resync', + }, + pathname: this.persistedDoc.path, + resync: true, + ts: TIMESTAMP, + doc_length: this.persistedDocContent.length, + }, + }, + ]) + }) + + it('handles overlapping ranges', async function () { + this.changes = [ + makeTrackedChange('ti1', { p: 0, i: 'the quic' }), + makeTrackedChange('td1', { p: 8, d: 'k br' }), + makeTrackedChange('ti2', { p: 14, hpos: 23, i: 'ps over' }), + ] + const updates = [ + docContentSyncUpdate(this.persistedDoc, this.persistedDocContent, { + changes: this.changes, + }), + ] + const expandedUpdates = + await this.SyncManager.promises.expandSyncUpdates( + this.projectId, + this.historyId, + updates, + this.extendLock + ) + + // Before: the [quick ][brown ] fox [jumps ]over the lazy dog + // After: [the quic][k br]own fox jum[ps over] the lazy dog + expect(expandedUpdates).to.deep.equal([ + { + doc: this.persistedDoc.doc, + op: [ + { + p: 0, + r: 'the quic', + tracking: { type: 'insert', userId: USER_ID, ts: TIMESTAMP }, + }, + { + p: 10, + r: 'br', + tracking: { type: 'delete', userId: USER_ID, ts: TIMESTAMP }, + }, + { + p: 12, + r: 'own ', + tracking: { type: 'none' }, + }, + { + p: 20, + r: 'jum', + tracking: { type: 'none' }, + }, + { + p: 23, + r: 'ps over', + tracking: { type: 'insert', userId: USER_ID, ts: TIMESTAMP }, + }, + { + p: 40, + r: 'dog', + tracking: { type: 'none' }, + }, + ], + meta: { + origin: { kind: 'history-resync' }, + pathname: this.persistedDoc.path, + resync: true, + ts: TIMESTAMP, + doc_length: this.persistedDocContent.length, + }, + }, + ]) + }) + }) }) })