From 7e8e2b058592faf0850a2a91bed99be95822c7d2 Mon Sep 17 00:00:00 2001 From: Domagoj Kriskovic Date: Mon, 17 Jun 2024 14:54:11 +0200 Subject: [PATCH] Send origin metadata through docupdater and project-history when restoring files (#18721) * add RestoreFileOrigin in overleaf-editor-core * support source to be an object * use sourceOrOrigin as param * rename to originOrSource so the priority is more clear * get timestamp from version * fix test * include version and min_count in getUpdatesFromHistory * extractOriginOrSource util function * fix RestoreManagerTests GitOrigin-RevId: 0ace05a6ade2794c753a9d0bffb4f858ecc6899a --- libraries/overleaf-editor-core/index.js | 2 + .../overleaf-editor-core/lib/origin/index.js | 4 ++ .../lib/origin/restore_file_origin.js | 62 +++++++++++++++++++ .../overleaf-editor-core/test/change.test.js | 25 ++++++++ .../app/js/DocumentManager.js | 12 +++- .../app/js/ProjectHistoryRedisManager.js | 26 ++++++-- services/document-updater/app/js/Utils.js | 18 ++++++ .../src/Features/History/RestoreManager.js | 26 ++++++-- .../features/history/services/types/shared.ts | 3 +- .../context/editor-manager-context.tsx | 2 +- .../unit/src/History/RestoreManagerTests.js | 21 ++++++- 11 files changed, 186 insertions(+), 15 deletions(-) create mode 100644 libraries/overleaf-editor-core/lib/origin/restore_file_origin.js diff --git a/libraries/overleaf-editor-core/index.js b/libraries/overleaf-editor-core/index.js index fe440badc5..e2c66ee7ae 100644 --- a/libraries/overleaf-editor-core/index.js +++ b/libraries/overleaf-editor-core/index.js @@ -22,6 +22,7 @@ const SetFileMetadataOperation = require('./lib/operation/set_file_metadata_oper const NoOperation = require('./lib/operation/no_operation') const Operation = require('./lib/operation') const RestoreOrigin = require('./lib/origin/restore_origin') +const RestoreFileOrigin = require('./lib/origin/restore_file_origin') const Origin = require('./lib/origin') const OtClient = require('./lib/ot_client') const TextOperation = require('./lib/operation/text_operation') @@ -66,6 +67,7 @@ exports.SetFileMetadataOperation = SetFileMetadataOperation exports.NoOperation = NoOperation exports.Operation = Operation exports.RestoreOrigin = RestoreOrigin +exports.RestoreFileOrigin = RestoreFileOrigin exports.Origin = Origin exports.OtClient = OtClient exports.TextOperation = TextOperation diff --git a/libraries/overleaf-editor-core/lib/origin/index.js b/libraries/overleaf-editor-core/lib/origin/index.js index 6ed663c242..2c00ca916c 100644 --- a/libraries/overleaf-editor-core/lib/origin/index.js +++ b/libraries/overleaf-editor-core/lib/origin/index.js @@ -5,6 +5,7 @@ const assert = require('check-types').assert // Dependencies are loaded at the bottom of the file to mitigate circular // dependency let RestoreOrigin = null +let RestoreFileOrigin = null /** * An Origin records where a {@link Change} came from. The Origin class handles @@ -31,6 +32,8 @@ class Origin { static fromRaw(raw) { if (!raw) return null if (raw.kind === RestoreOrigin.KIND) return RestoreOrigin.fromRaw(raw) + if (raw.kind === RestoreFileOrigin.KIND) + return RestoreFileOrigin.fromRaw(raw) return new Origin(raw.kind) } @@ -54,3 +57,4 @@ class Origin { module.exports = Origin RestoreOrigin = require('./restore_origin') +RestoreFileOrigin = require('./restore_file_origin') diff --git a/libraries/overleaf-editor-core/lib/origin/restore_file_origin.js b/libraries/overleaf-editor-core/lib/origin/restore_file_origin.js new file mode 100644 index 0000000000..6e42467b9c --- /dev/null +++ b/libraries/overleaf-editor-core/lib/origin/restore_file_origin.js @@ -0,0 +1,62 @@ +'use strict' + +const assert = require('check-types').assert + +const Origin = require('.') + +class RestoreFileOrigin extends Origin { + /** + * @param {number} version that was restored + * @param {string} path that was restored + * @param {Date} timestamp from the restored version + */ + constructor(version, path, timestamp) { + assert.integer(version, 'RestoreFileOrigin: bad version') + assert.string(path, 'RestoreFileOrigin: bad path') + assert.date(timestamp, 'RestoreFileOrigin: bad timestamp') + + super(RestoreFileOrigin.KIND) + this.version = version + this.path = path + this.timestamp = timestamp + } + + static fromRaw(raw) { + return new RestoreFileOrigin(raw.version, raw.path, new Date(raw.timestamp)) + } + + /** @inheritdoc */ + toRaw() { + return { + kind: RestoreFileOrigin.KIND, + version: this.version, + path: this.path, + timestamp: this.timestamp.toISOString(), + } + } + + /** + * @return {number} + */ + getVersion() { + return this.version + } + + /** + * @return {string} + */ + getPath() { + return this.path + } + + /** + * @return {Date} + */ + getTimestamp() { + return this.timestamp + } +} + +RestoreFileOrigin.KIND = 'file-restore' + +module.exports = RestoreFileOrigin diff --git a/libraries/overleaf-editor-core/test/change.test.js b/libraries/overleaf-editor-core/test/change.test.js index 9659110200..b4704fa25b 100644 --- a/libraries/overleaf-editor-core/test/change.test.js +++ b/libraries/overleaf-editor-core/test/change.test.js @@ -34,4 +34,29 @@ describe('Change', function () { expect(blobHashes.has(File.EMPTY_FILE_HASH)).to.be.true }) }) + + describe('RestoreFileOrigin', function () { + it('should convert to and from raw', function () { + const origin = new core.RestoreFileOrigin(1, 'path', new Date()) + const raw = origin.toRaw() + const newOrigin = core.Origin.fromRaw(raw) + expect(newOrigin).to.eql(origin) + }) + + it('change should have a correct origin class', function () { + const change = Change.fromRaw({ + operations: [], + timestamp: '2015-03-05T12:03:53.035Z', + authors: [null], + origin: { + kind: 'file-restore', + version: 1, + path: 'path', + timestamp: '2015-03-05T12:03:53.035Z', + }, + }) + + expect(change.getOrigin()).to.be.an.instanceof(core.RestoreFileOrigin) + }) + }) }) diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index 39832c8e7a..2ed9c2cfeb 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -8,6 +8,7 @@ const Metrics = require('./Metrics') const HistoryManager = require('./HistoryManager') const Errors = require('./Errors') const RangesManager = require('./RangesManager') +const { extractOriginOrSource } = require('./Utils') const MAX_UNFLUSHED_AGE = 300 * 1000 // 5 mins, document should be flushed to mongo this time after a change @@ -111,7 +112,7 @@ const DocumentManager = { } }, - async setDoc(projectId, docId, newLines, source, userId, undoing) { + async setDoc(projectId, docId, newLines, originOrSource, userId, undoing) { if (newLines == null) { throw new Error('No lines were provided to setDoc') } @@ -141,16 +142,23 @@ const DocumentManager = { o.u = true } // Turn on undo flag for each op for track changes } + + const { origin, source } = extractOriginOrSource(originOrSource) + const update = { doc: docId, op, v: version, meta: { type: 'external', - source, user_id: userId, }, } + if (origin) { + update.meta.origin = origin + } else if (source) { + update.meta.source = source + } // Keep track of external updates, whether they are for live documents // (flush) or unloaded documents (evict), and whether the update is a no-op. Metrics.inc('external-update', 1, { diff --git a/services/document-updater/app/js/ProjectHistoryRedisManager.js b/services/document-updater/app/js/ProjectHistoryRedisManager.js index d0802182ab..aa306893d5 100644 --- a/services/document-updater/app/js/ProjectHistoryRedisManager.js +++ b/services/document-updater/app/js/ProjectHistoryRedisManager.js @@ -9,7 +9,7 @@ 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 { addTrackedDeletesToContent, extractOriginOrSource } = require('./Utils') const HistoryConversions = require('./HistoryConversions') const OError = require('@overleaf/o-error') @@ -54,7 +54,7 @@ const ProjectHistoryRedisManager = { entityId, userId, projectUpdate, - source + originOrSource ) { projectUpdate = { pathname: projectUpdate.pathname, @@ -67,7 +67,15 @@ const ProjectHistoryRedisManager = { projectHistoryId, } projectUpdate[entityType] = entityId - if (source != null) { + + const { origin, source } = extractOriginOrSource(originOrSource) + + if (origin != null) { + projectUpdate.meta.origin = origin + if (origin.kind !== 'editor') { + projectUpdate.meta.type = 'external' + } + } else if (source != null) { projectUpdate.meta.source = source if (source !== 'editor') { projectUpdate.meta.type = 'external' @@ -90,7 +98,7 @@ const ProjectHistoryRedisManager = { entityId, userId, projectUpdate, - source + originOrSource ) { let docLines = projectUpdate.docLines let ranges @@ -117,7 +125,15 @@ const ProjectHistoryRedisManager = { projectUpdate.ranges = ranges } projectUpdate[entityType] = entityId - if (source != null) { + + const { origin, source } = extractOriginOrSource(originOrSource) + + if (origin != null) { + projectUpdate.meta.origin = origin + if (origin.kind !== 'editor') { + projectUpdate.meta.type = 'external' + } + } else if (source != null) { projectUpdate.meta.source = source if (source !== 'editor') { projectUpdate.meta.type = 'external' diff --git a/services/document-updater/app/js/Utils.js b/services/document-updater/app/js/Utils.js index 1655d9c779..496c40dbcc 100644 --- a/services/document-updater/app/js/Utils.js +++ b/services/document-updater/app/js/Utils.js @@ -83,10 +83,28 @@ function addTrackedDeletesToContent(content, trackedChanges) { return result } +/** + * checks if the given originOrSource should be treated as a source or origin + * TODO: remove this hack and remove all "source" references + */ +function extractOriginOrSource(originOrSource) { + let source = null + let origin = null + + if (typeof originOrSource === 'string') { + source = originOrSource + } else if (originOrSource && typeof originOrSource === 'object') { + origin = originOrSource + } + + return { source, origin } +} + module.exports = { isInsert, isDelete, isComment, addTrackedDeletesToContent, getDocLength, + extractOriginOrSource, } diff --git a/services/web/app/src/Features/History/RestoreManager.js b/services/web/app/src/Features/History/RestoreManager.js index 5f2c8244e6..cb69c5025f 100644 --- a/services/web/app/src/Features/History/RestoreManager.js +++ b/services/web/app/src/Features/History/RestoreManager.js @@ -48,7 +48,6 @@ const RestoreManager = { }, async revertFile(userId, projectId, version, pathname) { - const source = 'file-revert' const fsPath = await RestoreManager._writeFileVersionToDisk( projectId, version, @@ -71,6 +70,19 @@ const RestoreManager = { }) .catch(() => null) + const updates = await RestoreManager._getUpdatesFromHistory( + projectId, + version + ) + const updateAtVersion = updates.find(update => update.toV === version) + + const origin = { + kind: 'file-restore', + path: pathname, + version, + timestamp: new Date(updateAtVersion.meta.end_ts).toISOString(), + } + const importInfo = await FileSystemImportManager.promises.importFile( fsPath, pathname @@ -82,7 +94,7 @@ const RestoreManager = { basename, fsPath, file?.element?.linkedFileData, - source, + origin, userId ) @@ -101,7 +113,7 @@ const RestoreManager = { projectId, file.element._id, importInfo.type, - 'revert', + origin, userId ) } @@ -179,7 +191,7 @@ const RestoreManager = { basename, importInfo.lines, newRanges, - 'revert', + origin, userId ) }, @@ -226,6 +238,12 @@ const RestoreManager = { }/project/${projectId}/ranges/version/${version}/${encodeURIComponent(pathname)}` return await fetchJson(url) }, + + async _getUpdatesFromHistory(projectId, version) { + const url = `${Settings.apis.project_history.url}/project/${projectId}/updates?before=${version}&min_count=1` + const res = await fetchJson(url) + return res.updates + }, } module.exports = { ...callbackifyAll(RestoreManager), promises: RestoreManager } diff --git a/services/web/frontend/js/features/history/services/types/shared.ts b/services/web/frontend/js/features/history/services/types/shared.ts index 90962db115..105b2e287b 100644 --- a/services/web/frontend/js/features/history/services/types/shared.ts +++ b/services/web/frontend/js/features/history/services/types/shared.ts @@ -12,7 +12,7 @@ export interface Meta { start_ts: number end_ts: number type?: 'external' // TODO - source?: 'git-bridge' | 'file-revert' // TODO + source?: 'git-bridge' // TODO origin?: { kind: | 'dropbox' @@ -21,5 +21,6 @@ export interface Meta { | 'github' | 'history-resync' | 'history-migration' + | 'file-restore' } } diff --git a/services/web/frontend/js/features/ide-react/context/editor-manager-context.tsx b/services/web/frontend/js/features/ide-react/context/editor-manager-context.tsx index 064b8fae06..5272eb90c4 100644 --- a/services/web/frontend/js/features/ide-react/context/editor-manager-context.tsx +++ b/services/web/frontend/js/features/ide-react/context/editor-manager-context.tsx @@ -285,7 +285,7 @@ export const EditorManagerProvider: FC = ({ children }) => { ) { return } - if (update.meta.source === 'file-revert') { + if (update.meta.origin?.kind === 'file-restore') { return } showGenericMessageModal( diff --git a/services/web/test/unit/src/History/RestoreManagerTests.js b/services/web/test/unit/src/History/RestoreManagerTests.js index 11d34fc213..dc6d9e4d9e 100644 --- a/services/web/test/unit/src/History/RestoreManagerTests.js +++ b/services/web/test/unit/src/History/RestoreManagerTests.js @@ -286,6 +286,9 @@ describe('RestoreManager', function () { changes: this.tracked_changes, comments: this.comments, }) + this.RestoreManager.promises._getUpdatesFromHistory = sinon + .stub() + .resolves([{ toV: this.version, meta: { end_ts: Date.now() } }]) this.EditorController.promises.addDocWithRanges = sinon .stub() .resolves( @@ -360,7 +363,12 @@ describe('RestoreManager', function () { this.project_id, 'mock-file-id', 'doc', - 'revert', + { + kind: 'file-restore', + path: this.pathname, + version: this.version, + timestamp: new Date().toISOString(), + }, this.user_id ) }) @@ -389,7 +397,13 @@ describe('RestoreManager', function () { this.folder_id, 'foo.tex', ['foo', 'bar', 'baz'], - { changes: this.tracked_changes, comments: this.remappedComments } + { changes: this.tracked_changes, comments: this.remappedComments }, + { + kind: 'file-restore', + path: this.pathname, + version: this.version, + timestamp: new Date().toISOString(), + } ) }) @@ -418,6 +432,9 @@ describe('RestoreManager', function () { this.EditorController.promises.upsertFile = sinon .stub() .resolves({ _id: 'mock-file-id', type: 'file' }) + this.RestoreManager.promises._getUpdatesFromHistory = sinon + .stub() + .resolves([{ toV: this.version, meta: { end_ts: Date.now() } }]) }) it('should return the created entity if file exists', async function () {