diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index e97198ca05..0fa2958a20 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -1,4 +1,4 @@ -const { promisifyAll } = require('@overleaf/promise-utils') +const { callbackifyAll } = require('@overleaf/promise-utils') const RedisManager = require('./RedisManager') const ProjectHistoryRedisManager = require('./ProjectHistoryRedisManager') const PersistenceManager = require('./PersistenceManager') @@ -12,714 +12,445 @@ const RangesManager = require('./RangesManager') const MAX_UNFLUSHED_AGE = 300 * 1000 // 5 mins, document should be flushed to mongo this time after a change const DocumentManager = { - getDoc(projectId, docId, _callback) { - const timer = new Metrics.Timer('docManager.getDoc') - const callback = (...args) => { - timer.done() - _callback(...args) - } - - RedisManager.getDoc( - projectId, - docId, - ( - error, + async getDoc(projectId, docId) { + const { + lines, + version, + ranges, + pathname, + projectHistoryId, + unflushedTime, + historyRangesSupport, + } = await RedisManager.promises.getDoc(projectId, docId) + if (lines == null || version == null) { + logger.debug( + { projectId, docId }, + 'doc not in redis so getting from persistence API' + ) + const { + lines, + version, + ranges, + pathname, + projectHistoryId, + historyRangesSupport, + } = await PersistenceManager.promises.getDoc(projectId, docId) + logger.debug( + { + projectId, + docId, + lines, + version, + pathname, + projectHistoryId, + historyRangesSupport, + }, + 'got doc from persistence API' + ) + await RedisManager.promises.putDocInMemory( + projectId, + docId, + lines, + version, + ranges, + pathname, + projectHistoryId, + historyRangesSupport + ) + return { + lines, + version, + ranges: ranges || {}, + pathname, + projectHistoryId, + unflushedTime: null, + alreadyLoaded: false, + historyRangesSupport, + } + } else { + return { lines, version, ranges, pathname, projectHistoryId, unflushedTime, - lastUpdatedAt, - lastUpdatedBy, - historyRangesSupport - ) => { - if (error) { - return callback(error) - } - if (lines == null || version == null) { - logger.debug( - { projectId, docId }, - 'doc not in redis so getting from persistence API' - ) - PersistenceManager.getDoc( - projectId, - docId, - ( - error, - lines, - version, - ranges, - pathname, - projectHistoryId, - historyRangesSupport - ) => { - if (error) { - return callback(error) - } - logger.debug( - { - projectId, - docId, - lines, - version, - pathname, - projectHistoryId, - historyRangesSupport, - }, - 'got doc from persistence API' - ) - RedisManager.putDocInMemory( - projectId, - docId, - lines, - version, - ranges, - pathname, - projectHistoryId, - historyRangesSupport, - error => { - if (error) { - return callback(error) - } - callback( - null, - lines, - version, - ranges || {}, - pathname, - projectHistoryId, - null, - false, - historyRangesSupport - ) - } - ) - } - ) - } else { - callback( - null, - lines, - version, - ranges, - pathname, - projectHistoryId, - unflushedTime, - true, - historyRangesSupport - ) - } + alreadyLoaded: true, + historyRangesSupport, } - ) + } }, - getDocAndRecentOps(projectId, docId, fromVersion, _callback) { - const timer = new Metrics.Timer('docManager.getDocAndRecentOps') - const callback = (...args) => { - timer.done() - _callback(...args) - } + async getDocAndRecentOps(projectId, docId, fromVersion) { + const { lines, version, ranges, pathname, projectHistoryId } = + await DocumentManager.getDoc(projectId, docId) - DocumentManager.getDoc( - projectId, - docId, - (error, lines, version, ranges, pathname, projectHistoryId) => { - if (error) { - return callback(error) - } - if (fromVersion === -1) { - callback(null, lines, version, [], ranges, pathname, projectHistoryId) - } else { - RedisManager.getPreviousDocOps( - docId, - fromVersion, - version, - (error, ops) => { - if (error) { - return callback(error) - } - callback( - null, - lines, - version, - ops, - ranges, - pathname, - projectHistoryId - ) - } - ) - } + if (fromVersion === -1) { + return { lines, version, ops: [], ranges, pathname, projectHistoryId } + } else { + const ops = await RedisManager.promises.getPreviousDocOps( + docId, + fromVersion, + version + ) + return { + lines, + version, + ops, + ranges, + pathname, + projectHistoryId, } - ) + } }, - setDoc(projectId, docId, newLines, source, userId, undoing, _callback) { - const timer = new Metrics.Timer('docManager.setDoc') - const callback = (...args) => { - timer.done() - _callback(...args) - } - + async setDoc(projectId, docId, newLines, source, userId, undoing) { if (newLines == null) { - return callback(new Error('No lines were provided to setDoc')) + throw new Error('No lines were provided to setDoc') } const UpdateManager = require('./UpdateManager') - DocumentManager.getDoc( - projectId, - docId, - ( - error, - oldLines, - version, - ranges, - pathname, - projectHistoryId, - unflushedTime, - alreadyLoaded - ) => { - if (error) { - return callback(error) - } + const { + lines: oldLines, + version, + alreadyLoaded, + } = await DocumentManager.getDoc(projectId, docId) - if ( - oldLines != null && - oldLines.length > 0 && - oldLines[0].text != null - ) { - logger.debug( - { docId, projectId, oldLines, newLines }, - 'document is JSON so not updating' - ) - return callback(null) - } - - logger.debug( - { docId, projectId, oldLines, newLines }, - 'setting a document via http' - ) - const op = DiffCodec.diffAsShareJsOp(oldLines, newLines) - if (undoing) { - for (const o of op || []) { - o.u = true - } // Turn on undo flag for each op for track changes - } - const update = { - doc: docId, - op, - v: version, - meta: { - type: 'external', - source, - user_id: userId, - }, - } - // 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, { - status: op.length > 0 ? 'diff' : 'noop', - method: alreadyLoaded ? 'flush' : 'evict', - path: source, - }) - const applyUpdateIfNeeded = cb => { - if (op.length === 0) { - // Do not notify the frontend about a noop update. - // We still want to execute the callback code below - // to evict the doc if we loaded it into redis for - // this update, otherwise the doc would never be - // removed from redis. - return cb(null) - } - UpdateManager.applyUpdate(projectId, docId, update, cb) - } - applyUpdateIfNeeded(error => { - if (error) { - return callback(error) - } - // If the document was loaded already, then someone has it open - // in a project, and the usual flushing mechanism will happen. - // Otherwise we should remove it immediately since nothing else - // is using it. - if (alreadyLoaded) { - DocumentManager.flushDocIfLoaded( - projectId, - docId, - (error, result) => { - if (error) { - return callback(error) - } - callback(null, result) - } - ) - } else { - DocumentManager.flushAndDeleteDoc( - projectId, - docId, - {}, - (error, result) => { - // There is no harm in flushing project history if the previous - // call failed and sometimes it is required - HistoryManager.flushProjectChangesAsync(projectId) - - if (error) { - return callback(error) - } - callback(null, result) - } - ) - } - }) - } - ) - }, - - flushDocIfLoaded(projectId, docId, _callback) { - const timer = new Metrics.Timer('docManager.flushDocIfLoaded') - const callback = (...args) => { - timer.done() - _callback(...args) - } - RedisManager.getDoc( - projectId, - docId, - ( - error, - lines, - version, - ranges, - pathname, - projectHistoryId, - unflushedTime, - lastUpdatedAt, - lastUpdatedBy - ) => { - if (error) { - return callback(error) - } - if (lines == null || version == null) { - Metrics.inc('flush-doc-if-loaded', 1, { status: 'not-loaded' }) - logger.debug( - { projectId, docId }, - 'doc is not loaded so not flushing' - ) - // TODO: return a flag to bail out, as we go on to remove doc from memory? - callback(null) - } else if (unflushedTime == null) { - Metrics.inc('flush-doc-if-loaded', 1, { status: 'unmodified' }) - logger.debug( - { projectId, docId }, - 'doc is not modified so not flushing' - ) - callback(null) - } else { - logger.debug({ projectId, docId, version }, 'flushing doc') - Metrics.inc('flush-doc-if-loaded', 1, { status: 'modified' }) - PersistenceManager.setDoc( - projectId, - docId, - lines, - version, - ranges, - lastUpdatedAt, - lastUpdatedBy, - (error, result) => { - if (error) { - return callback(error) - } - RedisManager.clearUnflushedTime(docId, err => { - if (err) { - return callback(err) - } - callback(null, result) - }) - } - ) - } - } - ) - }, - - flushAndDeleteDoc(projectId, docId, options, _callback) { - const timer = new Metrics.Timer('docManager.flushAndDeleteDoc') - const callback = (...args) => { - timer.done() - _callback(...args) + if (oldLines != null && oldLines.length > 0 && oldLines[0].text != null) { + logger.debug( + { docId, projectId, oldLines, newLines }, + 'document is JSON so not updating' + ) + return } - DocumentManager.flushDocIfLoaded(projectId, docId, (error, result) => { - if (error) { - if (options.ignoreFlushErrors) { - logger.warn( - { projectId, docId, err: error }, - 'ignoring flush error while deleting document' - ) - } else { - return callback(error) - } - } - - RedisManager.removeDocFromMemory(projectId, docId, error => { - if (error) { - return callback(error) - } - callback(null, result) - }) + logger.debug( + { docId, projectId, oldLines, newLines }, + 'setting a document via http' + ) + const op = DiffCodec.diffAsShareJsOp(oldLines, newLines) + if (undoing) { + for (const o of op || []) { + o.u = true + } // Turn on undo flag for each op for track changes + } + const update = { + doc: docId, + op, + v: version, + meta: { + type: 'external', + source, + user_id: userId, + }, + } + // 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, { + status: op.length > 0 ? 'diff' : 'noop', + method: alreadyLoaded ? 'flush' : 'evict', + path: source, }) + + // Do not notify the frontend about a noop update. + // We still want to execute the code below + // to evict the doc if we loaded it into redis for + // this update, otherwise the doc would never be + // removed from redis. + if (op.length > 0) { + await UpdateManager.promises.applyUpdate(projectId, docId, update) + } + + // If the document was loaded already, then someone has it open + // in a project, and the usual flushing mechanism will happen. + // Otherwise we should remove it immediately since nothing else + // is using it. + if (alreadyLoaded) { + return await DocumentManager.flushDocIfLoaded(projectId, docId) + } else { + try { + return await DocumentManager.flushAndDeleteDoc(projectId, docId, {}) + } finally { + // There is no harm in flushing project history if the previous + // call failed and sometimes it is required + HistoryManager.flushProjectChangesAsync(projectId) + } + } }, - acceptChanges(projectId, docId, changeIds, _callback) { + async flushDocIfLoaded(projectId, docId) { + const { + lines, + version, + ranges, + unflushedTime, + lastUpdatedAt, + lastUpdatedBy, + } = await RedisManager.promises.getDoc(projectId, docId) + if (lines == null || version == null) { + Metrics.inc('flush-doc-if-loaded', 1, { status: 'not-loaded' }) + logger.debug({ projectId, docId }, 'doc is not loaded so not flushing') + // TODO: return a flag to bail out, as we go on to remove doc from memory? + return + } else if (unflushedTime == null) { + Metrics.inc('flush-doc-if-loaded', 1, { status: 'unmodified' }) + logger.debug({ projectId, docId }, 'doc is not modified so not flushing') + return + } + + logger.debug({ projectId, docId, version }, 'flushing doc') + Metrics.inc('flush-doc-if-loaded', 1, { status: 'modified' }) + const result = await PersistenceManager.promises.setDoc( + projectId, + docId, + lines, + version, + ranges, + lastUpdatedAt, + lastUpdatedBy + ) + await RedisManager.promises.clearUnflushedTime(docId) + return result + }, + + async flushAndDeleteDoc(projectId, docId, options) { + let result + try { + result = await DocumentManager.flushDocIfLoaded(projectId, docId) + } catch (error) { + if (options.ignoreFlushErrors) { + logger.warn( + { projectId, docId, err: error }, + 'ignoring flush error while deleting document' + ) + } else { + throw error + } + } + + await RedisManager.promises.removeDocFromMemory(projectId, docId) + return result + }, + + async acceptChanges(projectId, docId, changeIds) { if (changeIds == null) { changeIds = [] } - const timer = new Metrics.Timer('docManager.acceptChanges') - const callback = (...args) => { - timer.done() - _callback(...args) + + const { lines, version, ranges } = await DocumentManager.getDoc( + projectId, + docId + ) + if (lines == null || version == null) { + throw new Errors.NotFoundError(`document not found: ${docId}`) } - DocumentManager.getDoc( + const newRanges = RangesManager.acceptChanges(changeIds, ranges) + + await RedisManager.promises.updateDocument( projectId, docId, - (error, lines, version, ranges) => { - if (error) { - return callback(error) - } - if (lines == null || version == null) { - return callback( - new Errors.NotFoundError(`document not found: ${docId}`) - ) - } - - let newRanges - try { - newRanges = RangesManager.acceptChanges(changeIds, ranges) - } catch (err) { - return callback(err) - } - - RedisManager.updateDocument( - projectId, - docId, - lines, - version, - [], - newRanges, - {}, - error => { - if (error) { - return callback(error) - } - callback() - } - ) - } + lines, + version, + [], + newRanges, + {} ) }, - deleteComment(projectId, docId, commentId, userId, _callback) { - const timer = new Metrics.Timer('docManager.deleteComment') - const callback = (...args) => { - timer.done() - _callback(...args) + async deleteComment(projectId, docId, commentId, userId) { + const { lines, version, ranges, pathname, historyRangesSupport } = + await DocumentManager.getDoc(projectId, docId) + if (lines == null || version == null) { + throw new Errors.NotFoundError(`document not found: ${docId}`) } - DocumentManager.getDoc( + const newRanges = RangesManager.deleteComment(commentId, ranges) + + await RedisManager.promises.updateDocument( projectId, docId, - ( - error, - lines, - version, - ranges, - pathname, - projectHistoryId, - unflushedTime, - alreadyLoaded, - historyRangesSupport - ) => { - if (error) { - return callback(error) - } - if (lines == null || version == null) { - return callback( - new Errors.NotFoundError(`document not found: ${docId}`) - ) - } - - let newRanges - try { - newRanges = RangesManager.deleteComment(commentId, ranges) - } catch (err) { - return callback(err) - } - - RedisManager.updateDocument( - projectId, - docId, - lines, - version, - [], - newRanges, - {}, - error => { - if (error) { - return callback(error) - } - if (historyRangesSupport) { - ProjectHistoryRedisManager.queueOps( - projectId, - JSON.stringify({ - pathname, - deleteComment: commentId, - meta: { - ts: new Date(), - user_id: userId, - }, - }), - error => { - if (error) { - return callback(error) - } - callback() - } - ) - } else { - callback() - } - } - ) - } + lines, + version, + [], + newRanges, + {} ) + + if (historyRangesSupport) { + await ProjectHistoryRedisManager.promises.queueOps( + projectId, + JSON.stringify({ + pathname, + deleteComment: commentId, + meta: { + ts: new Date(), + user_id: userId, + }, + }) + ) + } }, - renameDoc(projectId, docId, userId, update, projectHistoryId, _callback) { - const timer = new Metrics.Timer('docManager.updateProject') - const callback = (...args) => { - timer.done() - _callback(...args) - } - - RedisManager.renameDoc( + async renameDoc(projectId, docId, userId, update, projectHistoryId) { + await RedisManager.promises.renameDoc( projectId, docId, userId, update, - projectHistoryId, - callback + projectHistoryId ) }, - getDocAndFlushIfOld(projectId, docId, callback) { - DocumentManager.getDoc( - projectId, - docId, - ( - error, - lines, - version, - ranges, - pathname, - projectHistoryId, - unflushedTime, - alreadyLoaded - ) => { - if (error) { - return callback(error) - } - // if doc was already loaded see if it needs to be flushed - if ( - alreadyLoaded && - unflushedTime != null && - Date.now() - unflushedTime > MAX_UNFLUSHED_AGE - ) { - DocumentManager.flushDocIfLoaded(projectId, docId, error => { - if (error) { - return callback(error) - } - callback(null, lines, version) - }) - } else { - callback(null, lines, version) - } - } - ) + async getDocAndFlushIfOld(projectId, docId) { + const { lines, version, unflushedTime, alreadyLoaded } = + await DocumentManager.getDoc(projectId, docId) + + // if doc was already loaded see if it needs to be flushed + if ( + alreadyLoaded && + unflushedTime != null && + Date.now() - unflushedTime > MAX_UNFLUSHED_AGE + ) { + await DocumentManager.flushDocIfLoaded(projectId, docId) + } + + return { lines, version } }, - resyncDocContents(projectId, docId, path, callback) { + async resyncDocContents(projectId, docId, path) { logger.debug({ projectId, docId, path }, 'start resyncing doc contents') - RedisManager.getDoc( + let { lines, version, projectHistoryId } = + await RedisManager.promises.getDoc(projectId, docId) + + // To avoid issues where the same docId appears with different paths, + // we use the path from the resyncProjectStructure update. If we used + // the path from the getDoc call to web then the two occurences of the + // docId would map to the same path, and this would be rejected by + // project-history as an unexpected resyncDocContent update. + if (lines == null || version == null) { + logger.debug( + { projectId, docId }, + 'resyncing doc contents - not found in redis - retrieving from web' + ) + ;({ lines, version, projectHistoryId } = + await PersistenceManager.promises.getDoc(projectId, docId, { + peek: true, + })) + } else { + logger.debug( + { projectId, docId }, + 'resyncing doc contents - doc in redis - will queue in redis' + ) + } + + await ProjectHistoryRedisManager.promises.queueResyncDocContent( projectId, + projectHistoryId, docId, - (error, lines, version, ranges, pathname, projectHistoryId) => { - if (error) { - return callback(error) - } - // To avoid issues where the same docId appears with different paths, - // we use the path from the resyncProjectStructure update. If we used - // the path from the getDoc call to web then the two occurences of the - // docId would map to the same path, and this would be rejected by - // project-history as an unexpected resyncDocContent update. - if (lines == null || version == null) { - logger.debug( - { projectId, docId }, - 'resyncing doc contents - not found in redis - retrieving from web' - ) - PersistenceManager.getDoc( - projectId, - docId, - { peek: true }, - (error, lines, version, ranges, pathname, projectHistoryId) => { - if (error) { - logger.error( - { projectId, docId, getDocError: error }, - 'resyncing doc contents - error retrieving from web' - ) - return callback(error) - } - ProjectHistoryRedisManager.queueResyncDocContent( - projectId, - projectHistoryId, - docId, - lines, - version, - path, // use the path from the resyncProjectStructure update - callback - ) - } - ) - } else { - logger.debug( - { projectId, docId }, - 'resyncing doc contents - doc in redis - will queue in redis' - ) - ProjectHistoryRedisManager.queueResyncDocContent( - projectId, - projectHistoryId, - docId, - lines, - version, - path, // use the path from the resyncProjectStructure update - callback - ) - } - } + lines, + version, + // use the path from the resyncProjectStructure update + path ) }, - getDocWithLock(projectId, docId, callback) { + async getDocWithLock(projectId, docId) { const UpdateManager = require('./UpdateManager') - UpdateManager.lockUpdatesAndDo( + return await UpdateManager.promises.lockUpdatesAndDo( DocumentManager.getDoc, projectId, - docId, - callback + docId ) }, - getDocAndRecentOpsWithLock(projectId, docId, fromVersion, callback) { + async getDocAndRecentOpsWithLock(projectId, docId, fromVersion) { const UpdateManager = require('./UpdateManager') - UpdateManager.lockUpdatesAndDo( + return await UpdateManager.promises.lockUpdatesAndDo( DocumentManager.getDocAndRecentOps, projectId, docId, - fromVersion, - callback + fromVersion ) }, - getDocAndFlushIfOldWithLock(projectId, docId, callback) { + async getDocAndFlushIfOldWithLock(projectId, docId) { const UpdateManager = require('./UpdateManager') - UpdateManager.lockUpdatesAndDo( + return await UpdateManager.promises.lockUpdatesAndDo( DocumentManager.getDocAndFlushIfOld, projectId, - docId, - callback + docId ) }, - setDocWithLock(projectId, docId, lines, source, userId, undoing, callback) { + async setDocWithLock(projectId, docId, lines, source, userId, undoing) { const UpdateManager = require('./UpdateManager') - UpdateManager.lockUpdatesAndDo( + return await UpdateManager.promises.lockUpdatesAndDo( DocumentManager.setDoc, projectId, docId, lines, source, userId, - undoing, - callback + undoing ) }, - flushDocIfLoadedWithLock(projectId, docId, callback) { + async flushDocIfLoadedWithLock(projectId, docId) { const UpdateManager = require('./UpdateManager') - UpdateManager.lockUpdatesAndDo( + return await UpdateManager.promises.lockUpdatesAndDo( DocumentManager.flushDocIfLoaded, projectId, - docId, - callback + docId ) }, - flushAndDeleteDocWithLock(projectId, docId, options, callback) { + async flushAndDeleteDocWithLock(projectId, docId, options) { const UpdateManager = require('./UpdateManager') - UpdateManager.lockUpdatesAndDo( + return await UpdateManager.promises.lockUpdatesAndDo( DocumentManager.flushAndDeleteDoc, projectId, docId, - options, - callback + options ) }, - acceptChangesWithLock(projectId, docId, changeIds, callback) { + async acceptChangesWithLock(projectId, docId, changeIds) { const UpdateManager = require('./UpdateManager') - UpdateManager.lockUpdatesAndDo( + await UpdateManager.promises.lockUpdatesAndDo( DocumentManager.acceptChanges, projectId, docId, - changeIds, - callback + changeIds ) }, - deleteCommentWithLock(projectId, docId, threadId, userId, callback) { + async deleteCommentWithLock(projectId, docId, threadId, userId) { const UpdateManager = require('./UpdateManager') - UpdateManager.lockUpdatesAndDo( + await UpdateManager.promises.lockUpdatesAndDo( DocumentManager.deleteComment, projectId, docId, threadId, - userId, - callback + userId ) }, - renameDocWithLock( - projectId, - docId, - userId, - update, - projectHistoryId, - callback - ) { + async renameDocWithLock(projectId, docId, userId, update, projectHistoryId) { const UpdateManager = require('./UpdateManager') - UpdateManager.lockUpdatesAndDo( + await UpdateManager.promises.lockUpdatesAndDo( DocumentManager.renameDoc, projectId, docId, userId, update, - projectHistoryId, - callback + projectHistoryId ) }, - resyncDocContentsWithLock(projectId, docId, path, callback) { + async resyncDocContentsWithLock(projectId, docId, path, callback) { const UpdateManager = require('./UpdateManager') - UpdateManager.lockUpdatesAndDo( + await UpdateManager.promises.lockUpdatesAndDo( DocumentManager.resyncDocContents, projectId, docId, @@ -729,46 +460,48 @@ const DocumentManager = { }, } -module.exports = DocumentManager -module.exports.promises = promisifyAll(DocumentManager, { - multiResult: { - getDoc: [ - 'lines', - 'version', - 'ranges', - 'pathname', - 'projectHistoryId', - 'unflushedTime', - 'alreadyLoaded', - 'historyRangesSupport', - ], - getDocWithLock: [ - 'lines', - 'version', - 'ranges', - 'pathname', - 'projectHistoryId', - 'unflushedTime', - 'alreadyLoaded', - 'historyRangesSupport', - ], - getDocAndFlushIfOld: ['lines', 'version'], - getDocAndFlushIfOldWithLock: ['lines', 'version'], - getDocAndRecentOps: [ - 'lines', - 'version', - 'ops', - 'ranges', - 'pathname', - 'projectHistoryId', - ], - getDocAndRecentOpsWithLock: [ - 'lines', - 'version', - 'ops', - 'ranges', - 'pathname', - 'projectHistoryId', - ], - }, -}) +module.exports = { + ...callbackifyAll(DocumentManager, { + multiResult: { + getDoc: [ + 'lines', + 'version', + 'ranges', + 'pathname', + 'projectHistoryId', + 'unflushedTime', + 'alreadyLoaded', + 'historyRangesSupport', + ], + getDocWithLock: [ + 'lines', + 'version', + 'ranges', + 'pathname', + 'projectHistoryId', + 'unflushedTime', + 'alreadyLoaded', + 'historyRangesSupport', + ], + getDocAndFlushIfOld: ['lines', 'version'], + getDocAndFlushIfOldWithLock: ['lines', 'version'], + getDocAndRecentOps: [ + 'lines', + 'version', + 'ops', + 'ranges', + 'pathname', + 'projectHistoryId', + ], + getDocAndRecentOpsWithLock: [ + 'lines', + 'version', + 'ops', + 'ranges', + 'pathname', + 'projectHistoryId', + ], + }, + }), + promises: DocumentManager, +} diff --git a/services/document-updater/app/js/UpdateManager.js b/services/document-updater/app/js/UpdateManager.js index 26d1396429..5e4b8abc7c 100644 --- a/services/document-updater/app/js/UpdateManager.js +++ b/services/document-updater/app/js/UpdateManager.js @@ -231,8 +231,6 @@ const UpdateManager = { } }, - // lockUpdatesAndDo can't be promisified yet because it expects a - // callback-style function async lockUpdatesAndDo(method, projectId, docId, ...args) { const profile = new Profiler('lockUpdatesAndDo', { project_id: projectId, @@ -242,21 +240,12 @@ const UpdateManager = { const lockValue = await LockManager.promises.getLock(docId) profile.log('getLock') - let responseArgs + let result try { await UpdateManager.processOutstandingUpdates(projectId, docId) profile.log('processOutstandingUpdates') - // TODO: method is still a callback-style function. Change this when promisifying DocumentManager - responseArgs = await new Promise((resolve, reject) => { - method(projectId, docId, ...args, (error, ...responseArgs) => { - if (error) { - reject(error) - } else { - resolve(responseArgs) - } - }) - }) + result = await method(projectId, docId, ...args) profile.log('method') } finally { await LockManager.promises.releaseLock(docId, lockValue) @@ -277,7 +266,7 @@ const UpdateManager = { } ) - return responseArgs + return result }, _sanitizeUpdate(update) { @@ -380,29 +369,4 @@ const UpdateManager = { }, } -const CallbackifiedUpdateManager = callbackifyAll(UpdateManager) - -module.exports = CallbackifiedUpdateManager -module.exports.promises = UpdateManager - -module.exports.lockUpdatesAndDo = function lockUpdatesAndDo( - method, - projectId, - docId, - ...rest -) { - const adjustedLength = Math.max(rest.length, 1) - const args = rest.slice(0, adjustedLength - 1) - const callback = rest[adjustedLength - 1] - - // TODO: During the transition to promises, UpdateManager.lockUpdatesAndDo - // returns the potentially multiple arguments that must be provided to the - // callback in an array. - UpdateManager.lockUpdatesAndDo(method, projectId, docId, ...args) - .then(responseArgs => { - callback(null, ...responseArgs) - }) - .catch(err => { - callback(err) - }) -} +module.exports = { ...callbackifyAll(UpdateManager), promises: UpdateManager } diff --git a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js index 258ab9e505..7648ecc4bb 100644 --- a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js +++ b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js @@ -1,4 +1,5 @@ const sinon = require('sinon') +const { expect } = require('chai') const modulePath = '../../../../app/js/DocumentManager.js' const SandboxedModule = require('sandboxed-module') const Errors = require('../../../../app/js/Errors') @@ -13,19 +14,55 @@ describe('DocumentManager', function () { } this.Metrics.Timer.prototype.done = sinon.stub() + this.RedisManager = { + promises: { + clearUnflushedTime: sinon.stub().resolves(), + getDoc: sinon.stub(), + getPreviousDocOps: sinon.stub(), + putDocInMemory: sinon.stub().resolves(), + removeDocFromMemory: sinon.stub().resolves(), + renameDoc: sinon.stub().resolves(), + updateDocument: sinon.stub().resolves(), + }, + } + this.ProjectHistoryRedisManager = { + promises: { + queueOps: sinon.stub().resolves(), + queueResyncDocContent: sinon.stub().resolves(), + }, + } + this.PersistenceManager = { + promises: { + getDoc: sinon.stub(), + setDoc: sinon.stub().resolves(), + }, + } + this.HistoryManager = { + flushProjectChangesAsync: sinon.stub(), + } + this.DiffCodec = { + diffAsShareJsOp: sinon.stub(), + } + this.UpdateManager = { + promises: { + applyUpdate: sinon.stub().resolves(), + }, + } + this.RangesManager = { + acceptChanges: sinon.stub(), + deleteComment: sinon.stub(), + } + this.DocumentManager = SandboxedModule.require(modulePath, { requires: { - './RedisManager': (this.RedisManager = {}), - './ProjectHistoryRedisManager': (this.ProjectHistoryRedisManager = {}), - './PersistenceManager': (this.PersistenceManager = {}), - './HistoryManager': (this.HistoryManager = { - flushProjectChangesAsync: sinon.stub(), - }), + './RedisManager': this.RedisManager, + './ProjectHistoryRedisManager': this.ProjectHistoryRedisManager, + './PersistenceManager': this.PersistenceManager, + './HistoryManager': this.HistoryManager, './Metrics': this.Metrics, - './RealTimeRedisManager': (this.RealTimeRedisManager = {}), - './DiffCodec': (this.DiffCodec = {}), - './UpdateManager': (this.UpdateManager = {}), - './RangesManager': (this.RangesManager = {}), + './DiffCodec': this.DiffCodec, + './UpdateManager': this.UpdateManager, + './RangesManager': this.RangesManager, './Errors': Errors, }, }) @@ -33,7 +70,6 @@ describe('DocumentManager', function () { this.projectHistoryId = 'history-id-123' this.doc_id = 'doc-id-123' this.user_id = 1234 - this.callback = sinon.stub() this.lines = ['one', 'two', 'three'] this.version = 42 this.ranges = { comments: 'mock', entries: 'mock' } @@ -51,72 +87,57 @@ describe('DocumentManager', function () { describe('flushAndDeleteDoc', function () { describe('successfully', function () { - beforeEach(function () { - this.RedisManager.removeDocFromMemory = sinon.stub().callsArg(2) - this.DocumentManager.flushDocIfLoaded = sinon.stub().callsArgWith(2) - this.DocumentManager.flushAndDeleteDoc( + beforeEach(async function () { + this.DocumentManager.promises.flushDocIfLoaded = sinon.stub().resolves() + await this.DocumentManager.promises.flushAndDeleteDoc( this.project_id, this.doc_id, - {}, - this.callback + {} ) }) it('should flush the doc', function () { - this.DocumentManager.flushDocIfLoaded + this.DocumentManager.promises.flushDocIfLoaded .calledWith(this.project_id, this.doc_id) .should.equal(true) }) it('should remove the doc from redis', function () { - this.RedisManager.removeDocFromMemory + this.RedisManager.promises.removeDocFromMemory .calledWith(this.project_id, this.doc_id) .should.equal(true) }) - - it('should call the callback without error', function () { - this.callback.calledWith(null).should.equal(true) - }) - - it('should time the execution', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) - }) }) describe('when a flush error occurs', function () { - beforeEach(function () { - this.DocumentManager.flushDocIfLoaded = sinon + beforeEach(async function () { + this.DocumentManager.promises.flushDocIfLoaded = sinon .stub() - .callsArgWith(2, new Error('boom!')) - this.RedisManager.removeDocFromMemory = sinon.stub().callsArg(2) + .rejects(new Error('boom!')) + await expect( + this.DocumentManager.promises.flushAndDeleteDoc( + this.project_id, + this.doc_id, + {} + ) + ).to.be.rejected }) - it('should not remove the doc from redis', function (done) { - this.DocumentManager.flushAndDeleteDoc( - this.project_id, - this.doc_id, - {}, - error => { - error.should.exist - this.RedisManager.removeDocFromMemory.called.should.equal(false) - done() - } + it('should not remove the doc from redis', function () { + this.RedisManager.promises.removeDocFromMemory.called.should.equal( + false ) }) describe('when ignoring flush errors', function () { - it('should remove the doc from redis', function (done) { - this.DocumentManager.flushAndDeleteDoc( + it('should remove the doc from redis', async function () { + await this.DocumentManager.promises.flushAndDeleteDoc( this.project_id, this.doc_id, - { ignoreFlushErrors: true }, - error => { - if (error) { - return done(error) - } - this.RedisManager.removeDocFromMemory.called.should.equal(true) - done() - } + { ignoreFlushErrors: true } + ) + this.RedisManager.promises.removeDocFromMemory.called.should.equal( + true ) }) }) @@ -125,40 +146,31 @@ describe('DocumentManager', function () { describe('flushDocIfLoaded', function () { describe('when the doc is in Redis', function () { - beforeEach(function () { - this.RedisManager.getDoc = sinon - .stub() - .callsArgWith( - 2, - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId, - this.unflushedTime, - this.lastUpdatedAt, - this.lastUpdatedBy - ) - this.RedisManager.clearUnflushedTime = sinon - .stub() - .callsArgWith(1, null) - this.PersistenceManager.setDoc = sinon.stub().yields() - this.DocumentManager.flushDocIfLoaded( + beforeEach(async function () { + this.RedisManager.promises.getDoc.resolves({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + unflushedTime: this.unflushedTime, + lastUpdatedAt: this.lastUpdatedAt, + lastUpdatedBy: this.lastUpdatedBy, + }) + await this.DocumentManager.promises.flushDocIfLoaded( this.project_id, - this.doc_id, - this.callback + this.doc_id ) }) it('should get the doc from redis', function () { - this.RedisManager.getDoc + this.RedisManager.promises.getDoc .calledWith(this.project_id, this.doc_id) .should.equal(true) }) it('should write the doc lines to the persistence layer', function () { - this.PersistenceManager.setDoc + this.PersistenceManager.promises.setDoc .calledWith( this.project_id, this.doc_id, @@ -170,242 +182,192 @@ describe('DocumentManager', function () { ) .should.equal(true) }) - - it('should call the callback without error', function () { - this.callback.calledWith(null).should.equal(true) - }) - - it('should time the execution', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) - }) }) describe('when the document is not in Redis', function () { - beforeEach(function () { - this.RedisManager.getDoc = sinon - .stub() - .callsArgWith(2, null, null, null, null) - this.PersistenceManager.setDoc = sinon.stub().yields() - this.DocumentManager.flushDocIfLoaded( + beforeEach(async function () { + this.RedisManager.promises.getDoc.resolves({ + lines: null, + version: null, + ranges: null, + }) + await this.DocumentManager.promises.flushDocIfLoaded( this.project_id, - this.doc_id, - this.callback + this.doc_id ) }) it('should get the doc from redis', function () { - this.RedisManager.getDoc + this.RedisManager.promises.getDoc .calledWith(this.project_id, this.doc_id) .should.equal(true) }) it('should not write anything to the persistence layer', function () { - this.PersistenceManager.setDoc.called.should.equal(false) - }) - - it('should call the callback without error', function () { - this.callback.calledWith(null).should.equal(true) - }) - - it('should time the execution', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) + this.PersistenceManager.promises.setDoc.called.should.equal(false) }) }) }) describe('getDocAndRecentOps', function () { describe('with a previous version specified', function () { - beforeEach(function () { - this.DocumentManager.getDoc = sinon - .stub() - .callsArgWith( - 2, - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId - ) - this.RedisManager.getPreviousDocOps = sinon - .stub() - .callsArgWith(3, null, this.ops) - this.DocumentManager.getDocAndRecentOps( + beforeEach(async function () { + this.DocumentManager.promises.getDoc = sinon.stub().resolves({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + }) + this.RedisManager.promises.getPreviousDocOps.resolves(this.ops) + this.result = await this.DocumentManager.promises.getDocAndRecentOps( this.project_id, this.doc_id, - this.fromVersion, - this.callback + this.fromVersion ) }) it('should get the doc', function () { - this.DocumentManager.getDoc + this.DocumentManager.promises.getDoc .calledWith(this.project_id, this.doc_id) .should.equal(true) }) it('should get the doc ops', function () { - this.RedisManager.getPreviousDocOps + this.RedisManager.promises.getPreviousDocOps .calledWith(this.doc_id, this.fromVersion, this.version) .should.equal(true) }) - it('should call the callback with the doc info', function () { - this.callback - .calledWith( - null, - this.lines, - this.version, - this.ops, - this.ranges, - this.pathname, - this.projectHistoryId - ) - .should.equal(true) - }) - - it('should time the execution', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) + it('should return the doc info', function () { + expect(this.result).to.deep.equal({ + lines: this.lines, + version: this.version, + ops: this.ops, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + }) }) }) describe('with no previous version specified', function () { - beforeEach(function () { - this.DocumentManager.getDoc = sinon - .stub() - .callsArgWith( - 2, - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId - ) - this.RedisManager.getPreviousDocOps = sinon - .stub() - .callsArgWith(3, null, this.ops) - this.DocumentManager.getDocAndRecentOps( + beforeEach(async function () { + this.DocumentManager.promises.getDoc = sinon.stub().resolves({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + }) + this.RedisManager.promises.getPreviousDocOps.resolves(this.ops) + this.result = await this.DocumentManager.promises.getDocAndRecentOps( this.project_id, this.doc_id, - -1, - this.callback + -1 ) }) it('should get the doc', function () { - this.DocumentManager.getDoc + this.DocumentManager.promises.getDoc .calledWith(this.project_id, this.doc_id) .should.equal(true) }) it('should not need to get the doc ops', function () { - this.RedisManager.getPreviousDocOps.called.should.equal(false) + this.RedisManager.promises.getPreviousDocOps.called.should.equal(false) }) - it('should call the callback with the doc info', function () { - this.callback - .calledWith( - null, - this.lines, - this.version, - [], - this.ranges, - this.pathname, - this.projectHistoryId - ) - .should.equal(true) - }) - - it('should time the execution', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) + it('should return the doc info', function () { + expect(this.result).to.deep.equal({ + lines: this.lines, + version: this.version, + ops: [], + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + }) }) }) }) describe('getDoc', function () { describe('when the doc exists in Redis', function () { - beforeEach(function () { - this.RedisManager.getDoc = sinon - .stub() - .callsArgWith( - 2, - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId, - this.unflushedTime, - this.lastUpdatedAt, - this.lastUpdatedBy, - this.historyRangesSupport - ) - this.DocumentManager.getDoc(this.project_id, this.doc_id, this.callback) + beforeEach(async function () { + this.RedisManager.promises.getDoc.resolves({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + unflushedTime: this.unflushedTime, + lastUpdatedAt: this.lastUpdatedAt, + lastUpdatedBy: this.lastUpdatedBy, + historyRangesSupport: this.historyRangesSupport, + }) + this.result = await this.DocumentManager.promises.getDoc( + this.project_id, + this.doc_id + ) }) it('should get the doc from Redis', function () { - this.RedisManager.getDoc + this.RedisManager.promises.getDoc .calledWith(this.project_id, this.doc_id) .should.equal(true) }) - it('should call the callback with the doc info', function () { - this.callback - .calledWith( - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId, - this.unflushedTime, - true, - this.historyRangesSupport - ) - .should.equal(true) - }) - - it('should time the execution', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) + it('should return the doc info', function () { + expect(this.result).to.deep.equal({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + unflushedTime: this.unflushedTime, + alreadyLoaded: true, + historyRangesSupport: this.historyRangesSupport, + }) }) }) describe('when the doc does not exist in Redis', function () { - beforeEach(function () { - this.RedisManager.getDoc = sinon - .stub() - .callsArgWith(2, null, null, null, null, null, null) - this.PersistenceManager.getDoc = sinon - .stub() - .callsArgWith( - 2, - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId, - this.historyRangesSupport - ) - this.RedisManager.putDocInMemory = sinon.stub().yields() - this.DocumentManager.getDoc(this.project_id, this.doc_id, this.callback) + beforeEach(async function () { + this.RedisManager.promises.getDoc.resolves({ + lines: null, + version: null, + ranges: null, + pathname: null, + projectHistoryId: null, + }) + this.PersistenceManager.promises.getDoc.resolves({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + historyRangesSupport: this.historyRangesSupport, + }) + this.result = await this.DocumentManager.promises.getDoc( + this.project_id, + this.doc_id + ) }) it('should try to get the doc from Redis', function () { - this.RedisManager.getDoc + this.RedisManager.promises.getDoc .calledWith(this.project_id, this.doc_id) .should.equal(true) }) it('should get the doc from the PersistenceManager', function () { - this.PersistenceManager.getDoc + this.PersistenceManager.promises.getDoc .calledWith(this.project_id, this.doc_id) .should.equal(true) }) it('should set the doc in Redis', function () { - this.RedisManager.putDocInMemory + this.RedisManager.promises.putDocInMemory .calledWith( this.project_id, this.doc_id, @@ -419,24 +381,17 @@ describe('DocumentManager', function () { .should.equal(true) }) - it('should call the callback with the doc info', function () { - this.callback - .calledWith( - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId, - null, - false, - this.historyRangesSupport - ) - .should.equal(true) - }) - - it('should time the execution', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) + it('should return doc info', function () { + expect(this.result).to.deep.equal({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + unflushedTime: null, + alreadyLoaded: false, + historyRangesSupport: this.historyRangesSupport, + }) }) }) }) @@ -450,53 +405,46 @@ describe('DocumentManager', function () { { i: 'foo', p: 4 }, { d: 'bar', p: 42 }, ] - this.DocumentManager.getDoc = sinon + this.DocumentManager.promises.getDoc = sinon.stub().resolves({ + lines: this.beforeLines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + unflushedTime: this.unflushedTime, + alreadyLoaded: true, + }) + this.DiffCodec.diffAsShareJsOp.returns(this.ops) + this.DocumentManager.promises.flushDocIfLoaded = sinon.stub().resolves() + this.DocumentManager.promises.flushAndDeleteDoc = sinon .stub() - .callsArgWith( - 2, - null, - this.beforeLines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId, - this.unflushedTime, - true - ) - this.DiffCodec.diffAsShareJsOp = sinon.stub().returns(this.ops) - this.UpdateManager.applyUpdate = sinon.stub().callsArgWith(3, null) - this.DocumentManager.flushDocIfLoaded = sinon.stub().callsArg(2) - this.DocumentManager.flushAndDeleteDoc = sinon.stub().callsArg(3) + .resolves() }) describe('when not loaded but with the same content', function () { - beforeEach(function () { - this.DiffCodec.diffAsShareJsOp = sinon.stub().returns([]) - this.DocumentManager.getDoc = sinon - .stub() - .yields( - null, - this.beforeLines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId, - this.unflushedTime, - false - ) - this.DocumentManager.setDoc( + beforeEach(async function () { + this.DiffCodec.diffAsShareJsOp.returns([]) + this.DocumentManager.promises.getDoc = sinon.stub().resolves({ + lines: this.beforeLines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + unflushedTime: this.unflushedTime, + alreadyLoaded: false, + }) + await this.DocumentManager.promises.setDoc( this.project_id, this.doc_id, this.beforeLines, this.source, this.user_id, - false, - this.callback + false ) }) it('should not apply the diff as a ShareJS op', function () { - this.UpdateManager.applyUpdate.called.should.equal(false) + this.UpdateManager.promises.applyUpdate.called.should.equal(false) }) it('should increment the external update metric', function () { @@ -510,28 +458,27 @@ describe('DocumentManager', function () { }) it('should flush and delete the doc from redis', function () { - this.DocumentManager.flushAndDeleteDoc + this.DocumentManager.promises.flushAndDeleteDoc .calledWith(this.project_id, this.doc_id) .should.equal(true) }) }) describe('when already loaded with the same content', function () { - beforeEach(function () { - this.DiffCodec.diffAsShareJsOp = sinon.stub().returns([]) - this.DocumentManager.setDoc( + beforeEach(async function () { + this.DiffCodec.diffAsShareJsOp.returns([]) + await this.DocumentManager.promises.setDoc( this.project_id, this.doc_id, this.beforeLines, this.source, this.user_id, - false, - this.callback + false ) }) it('should not apply the diff as a ShareJS op', function () { - this.UpdateManager.applyUpdate.called.should.equal(false) + this.UpdateManager.promises.applyUpdate.called.should.equal(false) }) it('should increment the external update metric', function () { @@ -545,27 +492,26 @@ describe('DocumentManager', function () { }) it('should flush the doc to Mongo', function () { - this.DocumentManager.flushDocIfLoaded + this.DocumentManager.promises.flushDocIfLoaded .calledWith(this.project_id, this.doc_id) .should.equal(true) }) }) describe('when already loaded', function () { - beforeEach(function () { - this.DocumentManager.setDoc( + beforeEach(async function () { + await this.DocumentManager.promises.setDoc( this.project_id, this.doc_id, this.afterLines, this.source, this.user_id, - false, - this.callback + false ) }) it('should get the current doc lines', function () { - this.DocumentManager.getDoc + this.DocumentManager.promises.getDoc .calledWith(this.project_id, this.doc_id) .should.equal(true) }) @@ -577,7 +523,7 @@ describe('DocumentManager', function () { }) it('should apply the diff as a ShareJS op', function () { - this.UpdateManager.applyUpdate + this.UpdateManager.promises.applyUpdate .calledWith(this.project_id, this.doc_id, { doc: this.doc_id, v: this.version, @@ -602,7 +548,7 @@ describe('DocumentManager', function () { }) it('should flush the doc to Mongo', function () { - this.DocumentManager.flushDocIfLoaded + this.DocumentManager.promises.flushDocIfLoaded .calledWith(this.project_id, this.doc_id) .should.equal(true) }) @@ -612,42 +558,29 @@ describe('DocumentManager', function () { false ) }) - - it('should call the callback', function () { - this.callback.calledWith(null).should.equal(true) - }) - - it('should time the execution', function () { - this.Metrics.Timer.prototype.done.called.should.equal(true) - }) }) describe('when not already loaded', function () { - beforeEach(function () { - this.DocumentManager.getDoc = sinon - .stub() - .callsArgWith( - 2, - null, - this.beforeLines, - this.version, - this.pathname, - null, - false - ) - this.DocumentManager.setDoc( + beforeEach(async function () { + this.DocumentManager.promises.getDoc = sinon.stub().resolves({ + lines: this.beforeLines, + version: this.version, + pathname: this.pathname, + unflushedTime: null, + alreadyLoaded: false, + }) + await this.DocumentManager.promises.setDoc( this.project_id, this.doc_id, this.afterLines, this.source, this.user_id, - false, - this.callback + false ) }) it('should flush and delete the doc from the doc updater', function () { - this.DocumentManager.flushAndDeleteDoc + this.DocumentManager.promises.flushAndDeleteDoc .calledWith(this.project_id, this.doc_id, {}) .should.equal(true) }) @@ -670,43 +603,39 @@ describe('DocumentManager', function () { }) describe('without new lines', function () { - beforeEach(function () { - this.DocumentManager.setDoc( - this.project_id, - this.doc_id, - null, - this.source, - this.user_id, - false, - this.callback - ) - }) - - it('should return the callback with an error', function () { - this.callback.calledWith(new Error('No lines were passed to setDoc')) + beforeEach(async function () { + await expect( + this.DocumentManager.promises.setDoc( + this.project_id, + this.doc_id, + null, + this.source, + this.user_id, + false + ) + ).to.be.rejectedWith('No lines were provided to setDoc') }) it('should not try to get the doc lines', function () { - this.DocumentManager.getDoc.called.should.equal(false) + this.DocumentManager.promises.getDoc.called.should.equal(false) }) }) describe('with the undoing flag', function () { - beforeEach(function () { + beforeEach(async function () { // Copy ops so we don't interfere with other tests this.ops = [ { i: 'foo', p: 4 }, { d: 'bar', p: 42 }, ] - this.DiffCodec.diffAsShareJsOp = sinon.stub().returns(this.ops) - this.DocumentManager.setDoc( + this.DiffCodec.diffAsShareJsOp.returns(this.ops) + await this.DocumentManager.promises.setDoc( this.project_id, this.doc_id, this.afterLines, this.source, this.user_id, - true, - this.callback + true ) }) @@ -730,39 +659,38 @@ describe('DocumentManager', function () { this.lines = ['original', 'lines'] this.ranges = { entries: 'mock', comments: 'mock' } this.updated_ranges = { entries: 'updated', comments: 'updated' } - this.DocumentManager.getDoc = sinon - .stub() - .yields(null, this.lines, this.version, this.ranges) - this.RangesManager.acceptChanges = sinon - .stub() - .returns(this.updated_ranges) - this.RedisManager.updateDocument = sinon.stub().yields() + this.DocumentManager.promises.getDoc = sinon.stub().resolves({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + }) + this.RangesManager.acceptChanges.returns(this.updated_ranges) }) describe('successfully with a single change', function () { - beforeEach(function () { - this.DocumentManager.acceptChanges( + beforeEach(async function () { + await this.DocumentManager.promises.acceptChanges( this.project_id, this.doc_id, - [this.change_id], - this.callback + [this.change_id] ) }) it("should get the document's current ranges", function () { - this.DocumentManager.getDoc + this.DocumentManager.promises.getDoc .calledWith(this.project_id, this.doc_id) .should.equal(true) }) it('should apply the accept change to the ranges', function () { - this.RangesManager.acceptChanges - .calledWith([this.change_id], this.ranges) - .should.equal(true) + this.RangesManager.acceptChanges.should.have.been.calledWith( + [this.change_id], + this.ranges + ) }) it('should save the updated ranges', function () { - this.RedisManager.updateDocument + this.RedisManager.promises.updateDocument .calledWith( this.project_id, this.doc_id, @@ -774,19 +702,14 @@ describe('DocumentManager', function () { ) .should.equal(true) }) - - it('should call the callback', function () { - this.callback.called.should.equal(true) - }) }) describe('successfully with multiple changes', function () { - beforeEach(function () { - this.DocumentManager.acceptChanges( + beforeEach(async function () { + await this.DocumentManager.promises.acceptChanges( this.project_id, this.doc_id, - this.change_ids, - this.callback + this.change_ids ) }) @@ -798,26 +721,21 @@ describe('DocumentManager', function () { }) describe('when the doc is not found', function () { - beforeEach(function () { - this.DocumentManager.getDoc = sinon + beforeEach(async function () { + this.DocumentManager.promises.getDoc = sinon .stub() - .yields(null, null, null, null) - this.DocumentManager.acceptChanges( - this.project_id, - this.doc_id, - [this.change_id], - this.callback - ) + .resolves({ lines: null, version: null, ranges: null }) + await expect( + this.DocumentManager.promises.acceptChanges( + this.project_id, + this.doc_id, + [this.change_id] + ) + ).to.be.rejectedWith(Errors.NotFoundError) }) it('should not save anything', function () { - this.RedisManager.updateDocument.called.should.equal(false) - }) - - it('should call the callback with a not found error', function () { - this.callback - .calledWith(sinon.match.instanceOf(Errors.NotFoundError)) - .should.equal(true) + this.RedisManager.promises.updateDocument.called.should.equal(false) }) }) }) @@ -830,39 +748,31 @@ describe('DocumentManager', function () { this.ranges = { comments: ['one', 'two', 'three'] } this.updated_ranges = { comments: ['one', 'three'] } this.historyRangesSupport = true - this.DocumentManager.getDoc = sinon - .stub() - .yields( - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId, - Date.now() - 1e9, - true, - this.historyRangesSupport - ) - this.RangesManager.deleteComment = sinon - .stub() - .returns(this.updated_ranges) - this.RedisManager.updateDocument = sinon.stub().yields() - this.ProjectHistoryRedisManager.queueOps = sinon.stub().yields() + this.DocumentManager.promises.getDoc = sinon.stub().resolves({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + unflushedTime: Date.now() - 1e9, + alreadyLoaded: true, + historyRangesSupport: this.historyRangesSupport, + }) + this.RangesManager.deleteComment.returns(this.updated_ranges) }) describe('successfully', function () { - beforeEach(function () { - this.DocumentManager.deleteComment( + beforeEach(async function () { + await this.DocumentManager.promises.deleteComment( this.project_id, this.doc_id, this.comment_id, - this.user_id, - this.callback + this.user_id ) }) it("should get the document's current ranges", function () { - this.DocumentManager.getDoc + this.DocumentManager.promises.getDoc .calledWith(this.project_id, this.doc_id) .should.equal(true) }) @@ -874,7 +784,7 @@ describe('DocumentManager', function () { }) it('should save the updated ranges', function () { - this.RedisManager.updateDocument + this.RedisManager.promises.updateDocument .calledWith( this.project_id, this.doc_id, @@ -888,172 +798,151 @@ describe('DocumentManager', function () { }) it('should queue the delete comment operation', function () { - this.ProjectHistoryRedisManager.queueOps - .calledWith( - this.project_id, - JSON.stringify({ - pathname: this.pathname, - deleteComment: this.comment_id, - meta: { - ts: new Date(), - user_id: this.user_id, - }, - }), - sinon.match.func - ) - .should.equal(true) - }) - - it('should call the callback', function () { - this.callback.called.should.equal(true) + this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWith( + this.project_id, + JSON.stringify({ + pathname: this.pathname, + deleteComment: this.comment_id, + meta: { + ts: new Date(), + user_id: this.user_id, + }, + }) + ) }) }) describe('when the doc is not found', function () { - beforeEach(function () { - this.DocumentManager.getDoc = sinon + beforeEach(async function () { + this.DocumentManager.promises.getDoc = sinon .stub() - .yields(null, null, null, null) - this.DocumentManager.acceptChanges( - this.project_id, - this.doc_id, - [this.comment_id], - this.callback - ) + .resolves({ lines: null, version: null, ranges: null }) + await expect( + this.DocumentManager.promises.acceptChanges( + this.project_id, + this.doc_id, + [this.comment_id] + ) + ).to.be.rejectedWith(Errors.NotFoundError) }) it('should not save anything', function () { - this.RedisManager.updateDocument.called.should.equal(false) - }) - - it('should call the callback with a not found error', function () { - this.callback - .calledWith(sinon.match.instanceOf(Errors.NotFoundError)) - .should.equal(true) + this.RedisManager.promises.updateDocument.called.should.equal(false) }) }) }) describe('getDocAndFlushIfOld', function () { beforeEach(function () { - this.DocumentManager.flushDocIfLoaded = sinon.stub().callsArg(2) + this.DocumentManager.promises.flushDocIfLoaded = sinon.stub().resolves() }) describe('when the doc is in Redis', function () { describe('and has changes to be flushed', function () { - beforeEach(function () { - this.DocumentManager.getDoc = sinon - .stub() - .callsArgWith( - 2, - null, - this.lines, - this.version, - this.ranges, - this.projectHistoryId, - this.pathname, - Date.now() - 1e9, - true - ) - this.DocumentManager.getDocAndFlushIfOld( + beforeEach(async function () { + this.DocumentManager.promises.getDoc = sinon.stub().resolves({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + projectHistoryId: this.projectHistoryId, + pathname: this.pathname, + unflushedTime: Date.now() - 1e9, + alreadyLoaded: true, + }) + this.result = await this.DocumentManager.promises.getDocAndFlushIfOld( this.project_id, - this.doc_id, - this.callback + this.doc_id ) }) it('should get the doc', function () { - this.DocumentManager.getDoc + this.DocumentManager.promises.getDoc .calledWith(this.project_id, this.doc_id) .should.equal(true) }) it('should flush the doc', function () { - this.DocumentManager.flushDocIfLoaded + this.DocumentManager.promises.flushDocIfLoaded .calledWith(this.project_id, this.doc_id) .should.equal(true) }) - it('should call the callback with the lines and versions', function () { - this.callback - .calledWith(null, this.lines, this.version) - .should.equal(true) + it('should return the lines and versions', function () { + expect(this.result).to.deep.equal({ + lines: this.lines, + version: this.version, + }) }) }) describe("and has only changes that don't need to be flushed", function () { - beforeEach(function () { - this.DocumentManager.getDoc = sinon - .stub() - .callsArgWith( - 2, - null, - this.lines, - this.version, - this.ranges, - this.pathname, - Date.now() - 100, - true - ) - this.DocumentManager.getDocAndFlushIfOld( + beforeEach(async function () { + this.DocumentManager.promises.getDoc = sinon.stub().resolves({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + unflushedTime: Date.now() - 100, + alreadyLoaded: true, + }) + this.result = await this.DocumentManager.promises.getDocAndFlushIfOld( this.project_id, - this.doc_id, - this.callback + this.doc_id ) }) it('should get the doc', function () { - this.DocumentManager.getDoc + this.DocumentManager.promises.getDoc .calledWith(this.project_id, this.doc_id) .should.equal(true) }) it('should not flush the doc', function () { - this.DocumentManager.flushDocIfLoaded.called.should.equal(false) + this.DocumentManager.promises.flushDocIfLoaded.called.should.equal( + false + ) }) - it('should call the callback with the lines and versions', function () { - this.callback - .calledWith(null, this.lines, this.version) - .should.equal(true) + it('should return the lines and versions', function () { + expect(this.result).to.deep.equal({ + lines: this.lines, + version: this.version, + }) }) }) }) describe('when the doc is not in Redis', function () { - beforeEach(function () { - this.DocumentManager.getDoc = sinon - .stub() - .callsArgWith( - 2, - null, - this.lines, - this.version, - this.ranges, - null, - false - ) - this.DocumentManager.getDocAndFlushIfOld( + beforeEach(async function () { + this.DocumentManager.promises.getDoc = sinon.stub().resolves({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + alreadyLoaded: false, + }) + this.result = await this.DocumentManager.promises.getDocAndFlushIfOld( this.project_id, - this.doc_id, - this.callback + this.doc_id ) }) it('should get the doc', function () { - this.DocumentManager.getDoc + this.DocumentManager.promises.getDoc .calledWith(this.project_id, this.doc_id) .should.equal(true) }) it('should not flush the doc', function () { - this.DocumentManager.flushDocIfLoaded.called.should.equal(false) + this.DocumentManager.promises.flushDocIfLoaded.called.should.equal( + false + ) }) - it('should call the callback with the lines and versions', function () { - this.callback - .calledWith(null, this.lines, this.version) - .should.equal(true) + it('should return the lines and versions', function () { + expect(this.result).to.deep.equal({ + lines: this.lines, + version: this.version, + }) }) }) }) @@ -1061,23 +950,21 @@ describe('DocumentManager', function () { describe('renameDoc', function () { beforeEach(function () { this.update = 'some-update' - this.RedisManager.renameDoc = sinon.stub().yields() }) describe('successfully', function () { - beforeEach(function () { - this.DocumentManager.renameDoc( + beforeEach(async function () { + await this.DocumentManager.promises.renameDoc( this.project_id, this.doc_id, this.user_id, this.update, - this.projectHistoryId, - this.callback + this.projectHistoryId ) }) it('should rename the document', function () { - this.RedisManager.renameDoc + this.RedisManager.promises.renameDoc .calledWith( this.project_id, this.doc_id, @@ -1087,103 +974,86 @@ describe('DocumentManager', function () { ) .should.equal(true) }) - - it('should call the callback', function () { - this.callback.called.should.equal(true) - }) }) }) describe('resyncDocContents', function () { describe('when doc is loaded in redis', function () { - beforeEach(function () { + beforeEach(async function () { this.pathnameFromProjectStructureUpdate = '/foo/bar.tex' - this.RedisManager.getDoc = sinon - .stub() - .callsArgWith( - 2, - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId - ) - this.ProjectHistoryRedisManager.queueResyncDocContent = sinon.stub() - this.DocumentManager.resyncDocContents( + this.RedisManager.promises.getDoc.resolves({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + }) + await this.DocumentManager.promises.resyncDocContents( this.project_id, this.doc_id, - this.pathnameFromProjectStructureUpdate, - this.callback + this.pathnameFromProjectStructureUpdate ) }) it('gets the doc contents from redis', function () { - this.RedisManager.getDoc + this.RedisManager.promises.getDoc .calledWith(this.project_id, this.doc_id) .should.equal(true) }) it('queues a resync doc content update', function () { - this.ProjectHistoryRedisManager.queueResyncDocContent + this.ProjectHistoryRedisManager.promises.queueResyncDocContent .calledWith( this.project_id, this.projectHistoryId, this.doc_id, this.lines, this.version, - this.pathnameFromProjectStructureUpdate, - this.callback + this.pathnameFromProjectStructureUpdate ) .should.equal(true) }) }) describe('when doc is not loaded in redis', function () { - beforeEach(function () { + beforeEach(async function () { this.pathnameFromProjectStructureUpdate = '/foo/bar.tex' - this.RedisManager.getDoc = sinon.stub().callsArgWith(2, null) - this.PersistenceManager.getDoc = sinon - .stub() - .yields( - null, - this.lines, - this.version, - this.ranges, - this.pathname, - this.projectHistoryId - ) - this.ProjectHistoryRedisManager.queueResyncDocContent = sinon.stub() - this.DocumentManager.resyncDocContents( + this.RedisManager.promises.getDoc.resolves({}) + this.PersistenceManager.promises.getDoc.resolves({ + lines: this.lines, + version: this.version, + ranges: this.ranges, + pathname: this.pathname, + projectHistoryId: this.projectHistoryId, + }) + await this.DocumentManager.promises.resyncDocContents( this.project_id, this.doc_id, - this.pathnameFromProjectStructureUpdate, - this.callback + this.pathnameFromProjectStructureUpdate ) }) it('tries to get the doc contents from redis', function () { - this.RedisManager.getDoc + this.RedisManager.promises.getDoc .calledWith(this.project_id, this.doc_id) .should.equal(true) }) it('gets the doc contents from web', function () { - this.PersistenceManager.getDoc + this.PersistenceManager.promises.getDoc .calledWith(this.project_id, this.doc_id, { peek: true }) .should.equal(true) }) it('queues a resync doc content update', function () { - this.ProjectHistoryRedisManager.queueResyncDocContent + this.ProjectHistoryRedisManager.promises.queueResyncDocContent .calledWith( this.project_id, this.projectHistoryId, this.doc_id, this.lines, this.version, - this.pathnameFromProjectStructureUpdate, - this.callback + this.pathnameFromProjectStructureUpdate ) .should.equal(true) }) diff --git a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js index 3f90d101c9..84ea05ba91 100644 --- a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js +++ b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js @@ -705,12 +705,9 @@ describe('UpdateManager', function () { describe('lockUpdatesAndDo', function () { beforeEach(function () { - this.method = sinon - .stub() - .yields(null, this.response_arg1, this.response_arg2) + this.methodResult = 'method result' + this.method = sinon.stub().resolves(this.methodResult) this.arg1 = 'argument 1' - this.response_arg1 = 'response argument 1' - this.response_arg2 = 'response argument 2' }) describe('successfully', function () { @@ -749,10 +746,7 @@ describe('UpdateManager', function () { }) it('should return the method response arguments', function () { - expect(this.response).to.deep.equal([ - this.response_arg1, - this.response_arg2, - ]) + expect(this.response).to.equal(this.methodResult) }) it('should release the lock', function () { @@ -797,7 +791,7 @@ describe('UpdateManager', function () { this.UpdateManager.promises.processOutstandingUpdates = sinon .stub() .resolves() - this.method = sinon.stub().yields(this.error) + this.method = sinon.stub().rejects(this.error) await expect( this.UpdateManager.promises.lockUpdatesAndDo( this.method,