From ba60f885a4a23ef3085d370bdfa53d68f85c3b5f Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 30 Jan 2025 11:47:02 +0000 Subject: [PATCH] [project-history] fetch most recent chunk once when processing updates (#23261) GitOrigin-RevId: 08ddd1f953b0dbae0541a60b45fec0f88e0a1d06 --- .../overleaf-editor-core/lib/snapshot.js | 2 +- .../app/js/HistoryStoreManager.js | 4 +- .../project-history/app/js/SnapshotManager.js | 39 ++++++++++++++- .../project-history/app/js/SyncManager.js | 36 ++++++++++++-- .../app/js/UpdatesProcessor.js | 9 +++- .../scripts/debug_translate_updates.js | 1 + .../test/acceptance/js/SyncTests.js | 10 ---- .../unit/js/SyncManager/SyncManagerTests.js | 47 +++++++++++++++++-- .../UpdatesManager/UpdatesProcessorTests.js | 9 +++- 9 files changed, 132 insertions(+), 25 deletions(-) diff --git a/libraries/overleaf-editor-core/lib/snapshot.js b/libraries/overleaf-editor-core/lib/snapshot.js index d7cbe62ae9..c33b48f829 100644 --- a/libraries/overleaf-editor-core/lib/snapshot.js +++ b/libraries/overleaf-editor-core/lib/snapshot.js @@ -224,7 +224,7 @@ class Snapshot { * * @param {string} kind see {File#load} * @param {ReadonlyBlobStore} blobStore - * @return {Promise} an object where keys are the pathnames and + * @return {Promise>} an object where keys are the pathnames and * values are the files in the snapshot */ async loadFiles(kind, blobStore) { diff --git a/services/project-history/app/js/HistoryStoreManager.js b/services/project-history/app/js/HistoryStoreManager.js index 24a70a19b1..eff25c8663 100644 --- a/services/project-history/app/js/HistoryStoreManager.js +++ b/services/project-history/app/js/HistoryStoreManager.js @@ -78,7 +78,8 @@ export function getMostRecentVersion(projectId, historyId, callback) { err1 || err2, mostRecentVersion, projectStructureAndDocVersions, - lastChange + lastChange, + chunk ) }) ) @@ -572,6 +573,7 @@ function _requestHistoryService(options, callback) { } export const promises = { + /** @type {(projectId: string, historyId: string) => Promise<{chunk: import('overleaf-editor-core/lib/types.js').RawChunk}>} */ getMostRecentChunk: promisify(getMostRecentChunk), getChunkAtVersion: promisify(getChunkAtVersion), getMostRecentVersion: promisify(getMostRecentVersion), diff --git a/services/project-history/app/js/SnapshotManager.js b/services/project-history/app/js/SnapshotManager.js index fd94c64a7f..f699e834cc 100644 --- a/services/project-history/app/js/SnapshotManager.js +++ b/services/project-history/app/js/SnapshotManager.js @@ -277,8 +277,26 @@ async function _getSnapshotAtVersion(projectId, version) { return snapshot } +/** + * @param {string} projectId + * @param {string} historyId + * @return {Promise>} + */ async function getLatestSnapshotFiles(projectId, historyId) { - const { snapshot } = await getLatestSnapshot(projectId, historyId) + const data = await HistoryStoreManager.promises.getMostRecentChunk( + projectId, + historyId + ) + return await getLatestSnapshotFilesForChunk(historyId, data) +} + +/** + * @param {string} historyId + * @param {{chunk: import('overleaf-editor-core/lib/types.js').RawChunk}} chunk + * @return {Promise>} + */ +async function getLatestSnapshotFilesForChunk(historyId, chunk) { + const { snapshot } = getLatestSnapshotFromChunk(chunk) const snapshotFiles = await snapshot.loadFiles( 'lazy', HistoryStoreManager.getBlobStore(historyId) @@ -286,11 +304,24 @@ async function getLatestSnapshotFiles(projectId, historyId) { return snapshotFiles } +/** + * @param {string} projectId + * @param {string} historyId + * @return {Promise<{version: number, snapshot: import('overleaf-editor-core').Snapshot}>} + */ async function getLatestSnapshot(projectId, historyId) { const data = await HistoryStoreManager.promises.getMostRecentChunk( projectId, historyId ) + return getLatestSnapshotFromChunk(data) +} + +/** + * @param {{chunk: import('overleaf-editor-core/lib/types.js').RawChunk}} data + * @return {{version: number, snapshot: import('overleaf-editor-core').Snapshot}} + */ +function getLatestSnapshotFromChunk(data) { if (data == null || data.chunk == null) { throw new OError('undefined chunk') } @@ -397,11 +428,15 @@ const getFileSnapshotStreamCb = callbackify(getFileSnapshotStream) const getProjectSnapshotCb = callbackify(getProjectSnapshot) const getLatestSnapshotCb = callbackify(getLatestSnapshot) const getLatestSnapshotFilesCb = callbackify(getLatestSnapshotFiles) +const getLatestSnapshotFilesForChunkCb = callbackify( + getLatestSnapshotFilesForChunk +) const getRangesSnapshotCb = callbackify(getRangesSnapshot) const getFileMetadataSnapshotCb = callbackify(getFileMetadataSnapshot) const getPathsAtVersionCb = callbackify(getPathsAtVersion) export { + getLatestSnapshotFromChunk, getChangesSinceCb as getChangesSince, getChangesInChunkSinceCb as getChangesInChunkSince, getFileSnapshotStreamCb as getFileSnapshotStream, @@ -409,6 +444,7 @@ export { getFileMetadataSnapshotCb as getFileMetadataSnapshot, getLatestSnapshotCb as getLatestSnapshot, getLatestSnapshotFilesCb as getLatestSnapshotFiles, + getLatestSnapshotFilesForChunkCb as getLatestSnapshotFilesForChunk, getRangesSnapshotCb as getRangesSnapshot, getPathsAtVersionCb as getPathsAtVersion, } @@ -420,6 +456,7 @@ export const promises = { getProjectSnapshot, getLatestSnapshot, getLatestSnapshotFiles, + getLatestSnapshotFilesForChunk, getRangesSnapshot, getPathsAtVersion, getFileMetadataSnapshot, diff --git a/services/project-history/app/js/SyncManager.js b/services/project-history/app/js/SyncManager.js index ca3963b9d2..3865dd8bf9 100644 --- a/services/project-history/app/js/SyncManager.js +++ b/services/project-history/app/js/SyncManager.js @@ -178,9 +178,18 @@ async function skipUpdatesDuringSync(projectId, updates) { return { updates: filteredUpdates, syncState } } +/** + * @param {string} projectId + * @param {string} projectHistoryId + * @param {{chunk: import('overleaf-editor-core/lib/types.js').RawChunk}} mostRecentChunk + * @param {Array} updates + * @param {() => Promise} extendLock + * @return {Promise>} + */ async function expandSyncUpdates( projectId, projectHistoryId, + mostRecentChunk, updates, extendLock ) { @@ -195,10 +204,11 @@ async function expandSyncUpdates( const syncState = await _getResyncState(projectId) // compute the current snapshot from the most recent chunk - const snapshotFiles = await SnapshotManager.promises.getLatestSnapshotFiles( - projectId, - projectHistoryId - ) + const snapshotFiles = + await SnapshotManager.promises.getLatestSnapshotFilesForChunk( + projectHistoryId, + mostRecentChunk + ) // check if snapshot files are valid const invalidFiles = _.pickBy( @@ -1102,15 +1112,31 @@ const skipUpdatesDuringSyncCb = callbackifyMultiResult(skipUpdatesDuringSync, [ 'updates', 'syncState', ]) + +/** + * @param {string} projectId + * @param {string} projectHistoryId + * @param {{chunk: import('overleaf-editor-core/lib/types.js').RawChunk}} mostRecentChunk + * @param {Array} updates + * @param {() => void} extendLock + * @param {(err: Error | null, updates?: Array) => void} callback + */ const expandSyncUpdatesCb = ( projectId, projectHistoryId, + mostRecentChunk, updates, extendLock, callback ) => { const extendLockPromises = promisify(extendLock) - expandSyncUpdates(projectId, projectHistoryId, updates, extendLockPromises) + expandSyncUpdates( + projectId, + projectHistoryId, + mostRecentChunk, + updates, + extendLockPromises + ) .then(result => { callback(null, result) }) diff --git a/services/project-history/app/js/UpdatesProcessor.js b/services/project-history/app/js/UpdatesProcessor.js index ad0dbc816b..bbce79e0be 100644 --- a/services/project-history/app/js/UpdatesProcessor.js +++ b/services/project-history/app/js/UpdatesProcessor.js @@ -362,7 +362,13 @@ export function _processUpdates( _getMostRecentVersionWithDebug( projectId, projectHistoryId, - (error, baseVersion, projectStructureAndDocVersions) => { + ( + error, + baseVersion, + projectStructureAndDocVersions, + _lastChange, + mostRecentChunk + ) => { if (projectStructureAndDocVersions == null) { projectStructureAndDocVersions = { project: null, docs: {} } } @@ -377,6 +383,7 @@ export function _processUpdates( SyncManager.expandSyncUpdates( projectId, projectHistoryId, + mostRecentChunk, filteredUpdates, extendLock, cb diff --git a/services/project-history/scripts/debug_translate_updates.js b/services/project-history/scripts/debug_translate_updates.js index b3fdc31bd9..bb896371ba 100755 --- a/services/project-history/scripts/debug_translate_updates.js +++ b/services/project-history/scripts/debug_translate_updates.js @@ -25,6 +25,7 @@ function expandResyncProjectStructure(chunk, update) { SyncManager.expandSyncUpdates( projectId, 99999, // dummy history id + chunk, [update], cb => cb(), // extend lock (err, result) => { diff --git a/services/project-history/test/acceptance/js/SyncTests.js b/services/project-history/test/acceptance/js/SyncTests.js index 001c920e3e..085462c7e2 100644 --- a/services/project-history/test/acceptance/js/SyncTests.js +++ b/services/project-history/test/acceptance/js/SyncTests.js @@ -53,16 +53,6 @@ describe('Syncing with web and doc-updater', function () { }, }, }) - MockHistoryStore() - .get(`/api/projects/${historyId}/latest/history`) - .reply(200, { - chunk: { - startVersion: 0, - history: { - changes: [], - }, - }, - }) ProjectHistoryClient.initializeProject(historyId, done) }) }) diff --git a/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js b/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js index 31ecb10b0b..8d7f8f1496 100644 --- a/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js +++ b/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js @@ -114,7 +114,7 @@ describe('SyncManager', function () { this.SnapshotManager = { promises: { - getLatestSnapshotFiles: sinon.stub(), + getLatestSnapshotFilesForChunk: sinon.stub(), }, } @@ -492,6 +492,7 @@ describe('SyncManager', function () { _hash: 'abcde', } this.loadedSnapshotDoc = File.fromString(this.persistedDocContent) + this.mostRecentChunk = 'fake chunk' this.fileMap = { 'main.tex': { isEditable: sinon.stub().returns(true), @@ -517,7 +518,7 @@ describe('SyncManager', function () { .returns('another.tex') this.UpdateTranslator._convertPathname.withArgs('1.png').returns('1.png') this.UpdateTranslator._convertPathname.withArgs('2.png').returns('2.png') - this.SnapshotManager.promises.getLatestSnapshotFiles.resolves( + this.SnapshotManager.promises.getLatestSnapshotFilesForChunk.resolves( this.fileMap ) }) @@ -527,13 +528,14 @@ describe('SyncManager', function () { const expandedUpdates = await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) expect(expandedUpdates).to.equal(updates) - expect(this.SnapshotManager.promises.getLatestSnapshotFiles).to.not.have - .been.called + expect(this.SnapshotManager.promises.getLatestSnapshotFilesForChunk).to + .not.have.been.called expect(this.extendLock).to.not.have.been.called }) @@ -549,6 +551,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -562,6 +565,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -586,6 +590,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -621,6 +626,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -658,6 +664,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -699,6 +706,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -738,6 +746,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -774,6 +783,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -824,6 +834,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -875,6 +886,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -921,6 +933,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -964,6 +977,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -998,6 +1012,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1026,6 +1041,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1051,6 +1067,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1097,6 +1114,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1139,6 +1157,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1163,6 +1182,7 @@ describe('SyncManager', function () { this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1176,6 +1196,7 @@ describe('SyncManager', function () { this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1195,6 +1216,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1221,6 +1243,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1259,6 +1282,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1303,6 +1327,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1328,6 +1353,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1364,6 +1390,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1418,6 +1445,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1440,6 +1468,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1483,6 +1512,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1517,6 +1547,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1560,6 +1591,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1614,6 +1646,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1643,6 +1676,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1733,6 +1767,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1754,6 +1789,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1795,6 +1831,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1836,6 +1873,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) @@ -1911,6 +1949,7 @@ describe('SyncManager', function () { await this.SyncManager.promises.expandSyncUpdates( this.projectId, this.historyId, + this.mostRecentChunk, updates, this.extendLock ) diff --git a/services/project-history/test/unit/js/UpdatesManager/UpdatesProcessorTests.js b/services/project-history/test/unit/js/UpdatesManager/UpdatesProcessorTests.js index a5d2846cbc..adebf4d1b3 100644 --- a/services/project-history/test/unit/js/UpdatesManager/UpdatesProcessorTests.js +++ b/services/project-history/test/unit/js/UpdatesManager/UpdatesProcessorTests.js @@ -289,10 +289,14 @@ describe('UpdatesProcessor', function () { this.newSyncState = { resyncProjectStructure: false } this.extendLock = sinon.stub().yields() + this.mostRecentChunk = 'fake-chunk' this.HistoryStoreManager.getMostRecentVersion.yields( null, - this.mostRecentVersionInfo + this.mostRecentVersionInfo, + null, + '_lastChange', + this.mostRecentChunk ) this.SyncManager.skipUpdatesDuringSync.yields( null, @@ -300,7 +304,7 @@ describe('UpdatesProcessor', function () { this.newSyncState ) this.SyncManager.expandSyncUpdates.callsArgWith( - 4, + 5, null, this.expandedUpdates ) @@ -345,6 +349,7 @@ describe('UpdatesProcessor', function () { return this.SyncManager.expandSyncUpdates.should.have.been.calledWith( this.project_id, this.ol_project_id, + this.mostRecentChunk, this.filteredUpdates, this.extendLock )